Proposal: Unified Macro Definitions

[wangeguo]

Problem Statement

The current Pliron Declarative Syntax Specification presents the following pain points:

1. Inconsistent Syntax: Disparate syntax between operations/types/attributes (e.g. #[def_op] vs #[def_type]), appearing fragmented
2. Verbose Trait Derivation: Overly cumbersome macros like #[derive_op_interface_impl(...)]
3. Scattered Implementation: Field-level attributes mixed with implementation blocks (e.g. impl_verify_succ!)

Proposed Solution

This proposal addresses the above issues through:

• Adoption of standardized #[derive(Operation/Type/Attribute)] syntax
• Consolidation of all metadata within #[definition] attributes
• Support for direct field-level attribute application

Note: Established macro patterns from popular crates like serde and clap can serve as references to improve consistency.

Current vs Proposed Implementation Comparison

1. Operation Definition

Current Implementation:

#[format_op("`(`$0`)` region($0)")]
#[def_op("test.if_op")]
#[derive_op_interface_impl(OneOpdInterface, ZeroResultInterface, OneRegionInterface)]
struct IfOp {}
impl_verify_succ!(IfOp);

Proposed Implementation:

#[derive(Operation)]
#[definition(
    name = "test.if_op",
    format = "`(`$0`)` region($0)",
    interfaces = [OneOpd, ZeroResult, OneRegion],
    verifier = "succ"
)]
struct IfOp {}

2. Type Definition

Current Implementation:

#[format_type("`type` `{` $flags `}`")]
#[def_type("test.flags_type")]
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
struct FlagsType {
    flags: u32,
}
impl_verify_succ!(FlagsType);

Proposed Implementation:

#[derive(Type)]
#[definition(
    name = "test.flags_type",
    format = "`type` `{` $flags `}`",
    verifier = "succ"
)]
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
struct FlagsType {
    flags: u32,
}

3. Attribute Definition

Current Implementation:

#[format_attribute("`attr` `(` $value `)`")]
#[def_attribute("test.string_attr")]
#[derive(Debug, Clone, PartialEq, Eq)]
struct StringAttr {
    value: String,
}
impl_verify_succ!(StringAttr);

Proposed Implementation:

#[derive(Attribute)]
#[definition(
    name = "test.string_attr",
    format = "`attr` `(` $value `)`",
    verifier = "succ"
)]
#[derive(Debug, Clone, PartialEq, Eq)]
struct StringAttr {
    value: String,
}

Key Benefits

1. Consistency: Unified #[definition] pattern across all IR entities
2. Readability: Clear visibility of field roles within struct definitions
3. Maintainability: Centralized management of all metadata
4. Extensibility: Simplified addition of new attributes (e.g. docs = "...")

Implementation Notes

The above examples are illustrative only. The proposed solution may not fully correspond with actual macro definitions in the codebase. This proposal presents preliminary concepts requiring further design refinement for discussion purposes.

Regarding actual refactoring approaches, forward compatibility considerations suggest two options:

1. Complete refactoring through incremental sub-Pull Requests
2. Transitional implementation via a new pliron-macros module

The specific implementation approach remains open for discussion.

[vaivaswatha]

Thank you @wangeguo . I like what you propose here. It does make it all neat.

What about this minor change:

#[pliron_entity(
    entity = "Op", // or Type or Attribute
    name = "test.if_op",
    // The actual format string below is still entity dependent, with rules applicable as it is today.
    // So we will need to document that the same as it done today, explaining the directives etc.
    format = "`(`$0`)` region($0)",
    interfaces = [OneOpd, ZeroResult, OneRegion],
    verifier = "succ"
)]
struct IfOp {}

The advantage with this minor change is that:

1. Simpler implementation since, when you evaluate the #[definition] part (as per the original proposition), you don't need to have already processed the #[derive(..)] part, to know what kind of IR entity is being processed. And even if the #[derive(..)] has been processed, you need to mark somehow for the #[definition], indicating the kind of entity it is. Not impossible, but probably a bit convoluted.
2. Implementing this variant of your proposal should be non-invasive and straightforward. You don't need to remove the existing macros. Just expand this new one into those, and let the existing implementations take over.
3. We get to keep the existing macros in-case someone wants more granular control over what they're definition (I'm not sure how useful this would be in practise, but just stating it).

Note that the macros #[op_interface] and #[op_interface_impl] (and similarly for Type and Attributes) are not covered here and they'll be defined and used as they are today.

What do you think?

[wangeguo]

Thank you for your thoughtful response! I really appreciate you taking the time to consider the proposal and suggest improvements.

To clarify, my original suggestion was just that - a proposal for potential simplification. I fully acknowledge that your expertise with Rust macros and the current Pliron macro implementation far exceeds mine. Your suggested approach is excellent - it's clearly easier to implement and maintains better backward compatibility with minimal invasiveness.

I particularly like how it preserves the existing macros for cases where more granular control might be needed. The implementation strategy of expanding the new macro into existing ones is very sensible.

One minor naming suggestion, if I may: since we're already clearly specifying the entity type (Op, Type, or Attribute) in the attribute itself, perhaps we could use just #[pliron] instead of #[pliron_entity]? This would be more concise and help with brand consistency across the project. But this is just a small stylistic suggestion - I'm completely happy with whichever naming you prefer.

Overall, I think your solution is great and I'm excited to see it implemented!

[rahulmutt]

@vaivaswatha I can take this on next since I've messed with the macro code enough by now that I feel comfortable. The solution you suggested makes a lot of sense and I like how there's a default for most users while at the same time more controls for power users.

One suggestion I have is that we have a separate attribute macro per entity type to make the intent clear. Making the entity type as an another argument to a more generic macro means having to validate potentially different configuration options per entity type. For example, for types, you have the option to generate a get, which doesn't exist for the operations or attributes.

So I suggest three new macros #[pliron_type(..)], #[pliron_op(..)] and #[pliron_attr(..)]. Another suggestion for naming - ir_type, ir_op and ir_attr. Let me know what you think.

[vaivaswatha]

Thank you @rahulmutt.

One suggestion I have is that we have a separate attribute macro per entity type to make the intent clear. Making the entity type as an another argument to a more generic macro means having to validate potentially different configuration options per entity type. For example, for types, you have the option to generate a get, which doesn't exist for the operations or attributes.

This makes sense. 👍 .

So I suggest three new macros #[pliron_type(..)], #[pliron_op(..)] and #[pliron_attr(..)].

👍 . These are good.

This is what I have in mind: What do you think?

1. The existing macros will remain (conceptually) untouched. We can add a comment at the end of their documentation suggesting to use the unified macros instead.
2. You'll create a new file for the new unified macros.
3. These new unified macros need not (conceptually) touch the syntax tree at all. They will just look at their own attributes / body and expand into the existing macros.
4. All the existing code will work as-is (i.e., migration to the new unified macros is not compulsory).
5. Migrate most of the existing code to use the new macros. In particular, I think the tests in pliron-derive should continue to use the existing macros so that we have them for debugging if required. There will be additional tests that use the new macros.

[rahulmutt]

All of that sounds good and mostly what I had in mind as well, thanks for the feedback @vaivaswatha!

[vaivaswatha]

Implemented with #83