Skip to content

core: (attr assembly fmt) add core directive classes (pt1)#5900

Open
n-io wants to merge 1 commit into
mainfrom
nicolai/attr-asm-fmt-pt1-core-directives
Open

core: (attr assembly fmt) add core directive classes (pt1)#5900
n-io wants to merge 1 commit into
mainfrom
nicolai/attr-asm-fmt-pt1-core-directives

Conversation

@n-io
Copy link
Copy Markdown
Collaborator

@n-io n-io commented Apr 21, 2026

Adding foundational classes for a declarative assembly format system for ParametrizedAttribute types, mirroring the implementations for op assembly format.

The existing format directives are tightly wired to operations, so this PR proposes instead to have a separate hierararchy based on AttrFormatDirective to keep the systems separate which helped keep the implementation clean. A purposefully small set of simple directives is implemented (whitespace, punctuation, keyword, param variable). Some of these overlap with implementations for op directives, so I factored out _print_whitespace, _print_punctuation, and _print_keyword. The AttrFormatProgram and AttrFormatParser resemble what is implemented for operations.

This PR is the first in a stacked series to enable full support for attribute/type assembly_format:

Add the foundational classes for a declarative assembly format system
for ParametrizedAttribute types, mirroring the existing op-side system:
AttrFormatDirective ABC, structural directives (whitespace, punctuation,
keyword), ParameterVariable, AttrFormatProgram, and AttrFormatParser.
@n-io n-io self-assigned this Apr 21, 2026
@n-io n-io added the core xDSL core (ir, textual format, ...) label Apr 21, 2026
@n-io n-io marked this pull request as ready for review April 21, 2026 10:55
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 21, 2026

Codecov Report

❌ Patch coverage is 76.21622% with 44 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.20%. Comparing base (d875f35) to head (377ba90).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
xdsl/irdl/declarative_assembly_format.py 73.60% 24 Missing and 9 partials ⚠️
xdsl/irdl/declarative_assembly_format_parser.py 81.66% 6 Missing and 5 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5900      +/-   ##
==========================================
- Coverage   86.23%   86.20%   -0.03%     
==========================================
  Files         418      418              
  Lines       59893    60077     +184     
  Branches     6920     6950      +30     
==========================================
+ Hits        51649    51791     +142     
- Misses       6660     6689      +29     
- Partials     1584     1597      +13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@n-io n-io changed the title core: add core directive classes for attr declarative assembly format (pt1) core: (attr assembly fmt) add core directive classes (pt1) Apr 21, 2026
Comment on lines +46 to +49
def _attr_def_for(cls: type[ParametrizedAttribute]) -> ParamAttrDef:
"""Build a ParamAttrDef from a ParametrizedAttribute class."""
return ParamAttrDef.from_pyrdl(cls)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to re-parse it, let's just get the attr def inline?

Suggested change
def _attr_def_for(cls: type[ParametrizedAttribute]) -> ParamAttrDef:
"""Build a ParamAttrDef from a ParametrizedAttribute class."""
return ParamAttrDef.from_pyrdl(cls)
def _attr_def_for(cls: type[ParametrizedAttribute]) -> ParamAttrDef:
"""Build a ParamAttrDef from a ParametrizedAttribute class."""
return cls.get_irdl_definition()

Comment on lines +22 to +25
@irdl_attr_definition
class SimpleType(ParametrizedAttribute, TypeAttribute):
name = "test_af.simple"
value: IntegerType = param_def()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can think of a simpler type :)

Suggested change
@irdl_attr_definition
class SimpleType(ParametrizedAttribute, TypeAttribute):
name = "test_af.simple"
value: IntegerType = param_def()
@irdl_attr_definition
class SimpleType(ParametrizedAttribute, TypeAttribute):
name = "test_af.simple"

@superlopuh
Copy link
Copy Markdown
Member

I would love for this part of the codebase to have 100% code coverage. Could you please take a look at what's not covered and add tests for it?

Also, I still feel like this first PR is bigger than ideal. Could you please try removing anything to do with parameters to start with? In theory we can test independently parsing punctuation and whitespace.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core xDSL core (ir, textual format, ...)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants