misc: use VarConstraint instead of Annotated[ConstraintVar] in operation definitions #3264

superlopuh · 2024-10-08T22:29:03Z

Using Annotated in the way that we do is not well defined by the Python type system, this fixes the current misuse to get the number of Pylance errors to 0.

The current proposal is to keep both implementations working, and to deprecate the Annotated API at some point in the future.

review-notebook-app · 2024-10-08T22:29:08Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

codecov · 2024-10-08T22:34:37Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.99%. Comparing base (964834a) to head (c975ac7).
Report is 6 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3264   +/-   ##
=======================================
  Coverage   89.98%   89.99%           
=======================================
  Files         444      444           
  Lines       55631    55659   +28     
  Branches     5353     5353           
=======================================
+ Hits        50060    50088   +28     
+ Misses       4174     4169    -5     
- Partials     1397     1402    +5

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

alexarice

Very nice. Not a comprehensive review but I left some comments. Is the constr field a part of the ParametrizedAttr superclass?

alexarice · 2024-10-09T08:31:18Z

tests/irdl/test_declarative_assembly_format.py

@@ -1661,13 +1664,25 @@ class ParamOne(ParametrizedAttribute, TypeAttribute, Generic[_T]):
        p: ParameterDef[_T]
        q: ParameterDef[Attribute]

+        @classmethod


This seems overkill for this test?

I think of tests as another kind of documentation, I feel like they should follow the style of the project unless there's a good reason not to.

alexarice · 2024-10-09T08:33:40Z

tests/irdl/test_operation_definition.py

+
+    T: ClassVar[VarConstraint[IntegerType | IndexType]] = VarConstraint(
+        "T", base(IntegerType) | base(IndexType)
+    )


In general do we want these constraints to be a visible part of the operation interface? Would it be better to use _T instead?

This is probably not related to the PR but I was suggesting that these ClassVars containing the constraints could be private

Oh yeah I guess they could be, I tried to modify a minimal amount with these changes.

alexarice · 2024-10-09T08:35:38Z

tests/irdl/test_operation_definition.py

+    op = GenericConstraintVarOp.create(
+        operands=[index_operand], result_types=[i32], attributes={"attribute": i32}
+    )
+    with pytest.raises(DiagnosticException):


It would be nice for these to match on the error

tests/tblgen_to_py/test.py

xdsl/dialects/builtin.py

alexarice · 2024-10-09T08:40:29Z

xdsl/dialects/ltl.py

-        ConstraintVar("T"),
-    ]
+    T: ClassVar[VarConstraint[Attribute]] = VarConstraint(
+        "T", AnyOf([Sequence, Property, IntegerType(1, signedness=Signedness.SIGNLESS)])


Signedness.SINGLESS is the default I believe

xdsl/irdl/constraints.py

alexarice · 2024-10-09T08:42:33Z

xdsl/tools/tblgen_to_py.py

-                ParamAttrConstraint(
-                    IntegerAttr, (AnyAttr(), EqAttrConstraint(IndexType()))
-                )
+                IntegerAttr[IndexType].constr(type=IndexTypeConstr)


Nice, ignore the earlier comment

Pylance errors: 84 -> 74. Part of #3264

…raints/generic-constraint-var

math-fehr

🚢

math-fehr · 2024-10-10T08:42:04Z

tests/irdl/test_declarative_assembly_format.py

-        lhs = operand_def(ParamOne[T])
+        lhs = operand_def(ParamOne[Attribute].constr(p=T))


I do have to say, I really dislike the verbosity of this, it will make it harder for newcomers to get started I think.

Yeah it definitely got less pretty. One potential alternative is ParamOneConstr(p=T), would you like me to try that instead? It's one more thing to import, but potentially worth the effort.

I just tested it locally and that API works and makes Pyright happy, it really doesn't want you to omit the type parameter even if it could be inferred in this case. I'm still inclined towards this more verbose API for the time being, with the hope that Pyright gets smart enough at some point to drop the unnecessary type parameter. We could also file it as a bug.

microsoft/pyright#9190

What about making constr a staticmethod instead? I just tried it locally and my pyright at least seems happy?

it doesn't work for me

How do you feel about iterating on this after this is merged and Pyright is updated?

Fine with me

https://peps.python.org/pep-0696/ also potentially relevant for making this api nicer

Yep, I look forward to being able to use the new, shiny APIs. I'm not sure when it would be wise to drop support for older Pythons, according to this calendar 3.10 is supported for another couple of years: https://endoflife.date/python

…es (#3274) This makes the next step for Pyright updating easier, as we can't actually properly represent generics in the IRDL constraint system. Currently, the arith base class helpers work because the generics are all specified by the time the ops are "defined" with the annotation. But my proposed solution of replacing the Annotated with VarConstraints means that we need to define the constraint on a specific type already. My understanding is that this is not a functional change, only requiring some clients of the base classes to update the names that they refer to. Part of #3264 Note stacked PR. --------- Co-authored-by: Alex Rice <alexrice999@hotmail.co.uk>

…int-var

superlopuh added the misc Miscellaneous label Oct 8, 2024

superlopuh requested a review from math-fehr October 8, 2024 22:29

superlopuh self-assigned this Oct 8, 2024

alexarice reviewed Oct 9, 2024

View reviewed changes

Base automatically changed from sasha/misc/pyright-ignores to main October 9, 2024 12:06

superlopuh force-pushed the sasha/irdl/typed-constraints/generic-constraint-var branch from 171cec9 to 33bb59f Compare October 9, 2024 14:55

superlopuh marked this pull request as draft October 9, 2024 14:55

superlopuh changed the base branch from main to sasha/misc/llvm-arith-binop October 9, 2024 14:56

Base automatically changed from sasha/misc/llvm-arith-binop to main October 9, 2024 16:14

superlopuh force-pushed the sasha/irdl/typed-constraints/generic-constraint-var branch from 33bb59f to 947c06a Compare October 9, 2024 22:25

misc: add more .constr helpers and constraints

b41312d

superlopuh mentioned this pull request Oct 9, 2024

misc: add more .constr helpers and constraints #3273

Merged

superlopuh added 10 commits October 9, 2024 23:50

fix IntegerAttr.constr

b23e989

dialects: (arith) split generic binary op definition into specific ones

d749b21

wip

3433782

wip

c2f2b3d

wip 47

2e88b4e

wip 43

eb8ab82

wip 41

380c446

misc: ArithmeticBinOpBase is always on integers

4bba9c1

arith: split out SignlessIntegerBinaryOperation

ba86527

migrate mod_arith

1c2d964

superlopuh force-pushed the sasha/irdl/typed-constraints/generic-constraint-var branch from 83ef4b9 to 1c2d964 Compare October 9, 2024 23:33

superlopuh changed the base branch from main to sasha/misc/arith-split-ops October 9, 2024 23:33

superlopuh added a commit that referenced this pull request Oct 9, 2024

misc: add more .constr helpers and constraints (#3273)

170ce50

Pylance errors: 84 -> 74. Part of #3264

superlopuh mentioned this pull request Oct 9, 2024

dialects: (arith) split generic binary op definition into specific ones #3274

Merged

superlopuh changed the title ~~[WIP] misc: use VarConstraint instead of Annotated[ConstraintVar] in operation definitions~~ misc: use VarConstraint instead of Annotated[ConstraintVar] in operation definitions Oct 9, 2024

add explicit error messages in test

57e2da3

remove default signedness in constraint

92c2a01

superlopuh marked this pull request as ready for review October 9, 2024 23:45

superlopuh added 2 commits October 10, 2024 09:39

Merge branch 'main' into sasha/misc/arith-split-ops

c5bbe93

Merge branch 'sasha/misc/arith-split-ops' into sasha/irdl/typed-const…

f4a3350

…raints/generic-constraint-var

math-fehr approved these changes Oct 10, 2024

View reviewed changes

Base automatically changed from sasha/misc/arith-split-ops to main October 10, 2024 10:22

Merge branch 'main' into sasha/irdl/typed-constraints/generic-constra…

c975ac7

…int-var

superlopuh merged commit 187eb31 into main Oct 10, 2024
14 checks passed

superlopuh deleted the sasha/irdl/typed-constraints/generic-constraint-var branch October 10, 2024 21:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

misc: use VarConstraint instead of Annotated[ConstraintVar] in operation definitions #3264

misc: use VarConstraint instead of Annotated[ConstraintVar] in operation definitions #3264

superlopuh commented Oct 8, 2024 •

edited

Loading

review-notebook-app bot commented Oct 8, 2024

codecov bot commented Oct 8, 2024 •

edited

Loading

alexarice left a comment

alexarice Oct 9, 2024

superlopuh Oct 10, 2024

alexarice Oct 9, 2024

superlopuh Oct 9, 2024

alexarice Oct 10, 2024

superlopuh Oct 10, 2024

alexarice Oct 9, 2024

alexarice Oct 9, 2024

alexarice Oct 9, 2024

math-fehr left a comment

math-fehr Oct 10, 2024

superlopuh Oct 10, 2024

superlopuh Oct 10, 2024

superlopuh Oct 10, 2024

alexarice Oct 10, 2024

superlopuh Oct 10, 2024

superlopuh Oct 10, 2024

alexarice Oct 10, 2024

alexarice Oct 10, 2024

superlopuh Oct 10, 2024

		lhs = operand_def(ParamOne[T])
		lhs = operand_def(ParamOne[Attribute].constr(p=T))

misc: use VarConstraint instead of Annotated[ConstraintVar] in operation definitions #3264

misc: use VarConstraint instead of Annotated[ConstraintVar] in operation definitions #3264

Conversation

superlopuh commented Oct 8, 2024 • edited Loading

review-notebook-app bot commented Oct 8, 2024

codecov bot commented Oct 8, 2024 • edited Loading

Codecov Report

alexarice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

math-fehr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

superlopuh commented Oct 8, 2024 •

edited

Loading

codecov bot commented Oct 8, 2024 •

edited

Loading