dialects: (builtin) Fix documentation for parsing of bools, and add conversion to Python bool #3689

watermelonwolverine · 2025-01-02T15:02:25Z

Updated documentation in attribute_parser and base_parser regarding the integer values of bools.
Added to_bool to IntegerAttr
See issue #3684

…he integer values of booleans. Added `to_bool` to IntegerAttr (xdslproject#3684)

codecov · 2025-01-02T15:08:27Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.31%. Comparing base (d7f6068) to head (d05edd4).
Report is 13 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3689      +/-   ##
==========================================
+ Coverage   91.30%   91.31%   +0.01%     
==========================================
  Files         466      467       +1     
  Lines       58357    58493     +136     
  Branches     5624     5640      +16     
==========================================
+ Hits        53280    53411     +131     
- Misses       3628     3632       +4     
- Partials     1449     1450       +1

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

superlopuh · 2025-01-02T15:48:17Z

xdsl/dialects/builtin.py

+    def to_bool(self) -> bool:
+        # Do assertion, yes, no?
+        assert self.type == i1
+        return bool(self.value.data)


Suggested change

def to_bool(self) -> bool:

# Do assertion, yes, no?

assert self.type == i1

return bool(self.value.data)

def __bool__(self) -> bool:

"""Returns True if value is non-zero."""

return bool(self.value.data)

superlopuh · 2025-01-02T15:49:02Z

tests/dialects/test_builtin.py

+    with pytest.raises(AssertionError):
+        IntegerAttr(1, i8).to_bool()
+


Yeah I don't think we need to be this strict, I'm happy to follow C/Python's truthiness as being non-zero.

superlopuh · 2025-01-02T16:51:13Z

Why not __bool__?

watermelonwolverine · 2025-01-02T17:01:27Z

Huh, I didn't think about that. Could that mess with checks where one checks if an optional attribute/property is present?

E.g.:

if self.opt_bool_attr:
    # Do something when attribute present

I think a saw a few of those checks. 🤔

superlopuh · 2025-01-02T17:03:49Z

This is an antipattern and should never be used, for precisely this reason :) I would like to move towards more functions using attributes for calculations instead of python integers, and this is a valid use for them, I would argue that we want this functionality for them also. I would recommend making the change, and then waiting for @math-fehr and @alexarice's reviews before merging.

watermelonwolverine · 2025-01-02T22:02:09Z

This is an antipattern and should never be used, for precisely this reason :) I would like to move towards more functions using attributes for calculations instead of python integers, and this is a valid use for them, I would argue that we want this functionality for them also. I would recommend making the change, and then waiting for @math-fehr and @alexarice's reviews before merging.

A test failed because there was such an if. This could potentially break a lot of code of people who use xDSL.

alexarice · 2025-01-02T22:15:21Z

Whether or not it is an antipattern, I feel that we have used conversion to bool in place of "is not None" in a lot of places and I'm not sure there's a good way to find all the places.

math-fehr · 2025-01-02T22:39:33Z

I would personally not overload bool, and would probably name the function is_nonnull instead?

superlopuh · 2025-01-03T10:14:36Z

xdsl/dialects/builtin.py

+    def __bool__(self) -> bool:
+        """Returns True if value is non-zero."""
+        return bool(self.data)
+


would be good to add unit tests for this for good measure

superlopuh · 2025-01-03T10:18:41Z

@alexarice I've been pretty consistent in my PR reviews to discourage this approach, and I feel that we're actually in a good place here, modulo the two uses of it in csl.

@math-fehr, why not? I don't see why we shouldn't be able to use integer attrs as idiomatic Python integers, whether it's IntAttr or IntegerAttr.

alexarice · 2025-01-03T14:29:11Z

My memory is that we've changed a few if X is not None to if X in PRs, but maybe I'm misremembering. I still feel that bugs could creep in but am happy for the change to go ahead if this is the direction we want to go in.

superlopuh · 2025-01-03T14:53:39Z

If I had anything to do with it it would have been the other way around, which is hopefully why all the tests pass with this change.

watermelonwolverine · 2025-01-04T14:31:15Z

One thing you might want to consider when going with __bool__ is that there are some non-obvious pitfalls, for example you can't use:

if bool_attr is True:
    # Do stuff

or

if bool_attr is False:
    # Do stuff

You could argue that this code is simply wrong but it's a code a beginner might write (or someone who likes to make things very explicit). Basically the programmers would have to know when they can use a IntegerAttr like a bool and in what situations they shouldn't. Now, whether you really want to cater to beginner programmers is another question.

As another side note: What about adding a function is_present that checks if an optional attribute, region, operand is present? This would make things very explicit and would even open the door to changing the underlying implementation of optionals at some later point. I.e. using something other than None. At the moment it would simple check if the given value is not None.

superlopuh · 2025-01-05T12:33:26Z

Pyright should flag that as the types have no overlap, so I don't think it's a significant concern.

superlopuh · 2025-01-05T12:34:09Z

I'm not sure that it would be a good idea to add that helper, it's not really shorter or easier to understand than is not None

alexarice

Some small comments. If we're happy that we won't be introducing bugs, I'm happy with using __bool__ (and don't think we should have a helper for is not None though I can only see references to that in the discussion and not the code)

alexarice · 2025-01-06T10:45:04Z

tests/dialects/test_builtin.py

+
+
+def test_IntegerAttr___bool__():
+    assert not IntegerAttr.from_bool(False)


Suggested change

assert not IntegerAttr.from_bool(False)

assert not BoolAttr.from_bool(False)

Super nit and probably doesn't really matter

alexarice · 2025-01-06T10:45:15Z

tests/dialects/test_builtin.py

+
+def test_IntegerAttr___bool__():
+    assert not IntegerAttr.from_bool(False)
+    assert IntegerAttr.from_bool(True)


Suggested change

assert IntegerAttr.from_bool(True)

assert BoolAttr.from_bool(True)

alexarice · 2025-01-06T10:47:24Z

xdsl/parser/base_parser.py

@@ -45,7 +45,7 @@ def parse_optional_integer(
        """
        Parse an (possible negative) integer. The integer can either be
        decimal or hexadecimal.
-        Optionally allow parsing of 'true' or 'false' into 1 and 0.
+        Optionally allow parsing of 'true' or 'false' into -1 and 0.


This does not agree with the implementation below

It does though

Ah actually not here

You're right, all of the functions with the changed documentation here return the 1/0 values, it's only when creating the attribute that the value is different. This should be reflected in documentation for the attribute, not the parser.

Yes, I did not put a comment on the doc change for the attribute

alexarice · 2025-01-06T10:47:42Z

xdsl/parser/base_parser.py

@@ -78,7 +78,7 @@ def parse_integer(
        """
        Parse an (possible negative) integer. The integer can
        either be decimal or hexadecimal.
-        Optionally allow parsing of 'true' or 'false' into 1 and 0.
+        Optionally allow parsing of 'true' or 'false' into -1 and 0.


alexarice · 2025-01-06T10:48:29Z

xdsl/parser/base_parser.py

@@ -121,7 +121,7 @@ def parse_optional_number(
    ) -> int | float | None:
        """
        Parse a (possibly negative) integer or float literal, if present.
-        Can optionally parse 'true' or 'false' into 1 and 0.
+        Can optionally parse 'true' or 'false' into -1 and 0.


alexarice · 2025-01-06T10:48:42Z

xdsl/parser/base_parser.py

@@ -149,7 +149,7 @@ def parse_number(
    ) -> int | float:
        """
        Parse a (possibly negative) integer or float literal.
-        Can optionally parse 'true' or 'false' into 1 and 0.
+        Can optionally parse 'true' or 'false' into -1 and 0.


Updated documentation in attribute_parser and base_parser regarding t…

765a9e4

…he integer values of booleans. Added `to_bool` to IntegerAttr (xdslproject#3684)

superlopuh reviewed Jan 2, 2025

View reviewed changes

Implemented suggested changes

d8dc686

superlopuh assigned watermelonwolverine Jan 2, 2025

superlopuh added core xDSL core (ir, textual format, ...) dialects Changes on the dialects and removed core xDSL core (ir, textual format, ...) labels Jan 2, 2025

superlopuh changed the title ~~core: (Parsing) Fix for issue #3684~~ dialects: (builtin) Fix documentation for parsing of bools, and add conversion to Python bool Jan 2, 2025

superlopuh requested review from alexarice, math-fehr and jorendumoulin January 2, 2025 17:06

watermelonwolverine added 2 commits January 2, 2025 17:54

Implemented suggested changes

986605f

Fixed bug in print_csl.py

711157c

superlopuh reviewed Jan 3, 2025

View reviewed changes

Implemented suggested changes

d05edd4

alexarice reviewed Jan 6, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dialects: (builtin) Fix documentation for parsing of bools, and add conversion to Python bool #3689

dialects: (builtin) Fix documentation for parsing of bools, and add conversion to Python bool #3689

watermelonwolverine commented Jan 2, 2025

codecov bot commented Jan 2, 2025 •

edited

Loading

superlopuh Jan 2, 2025

superlopuh Jan 2, 2025

superlopuh commented Jan 2, 2025

watermelonwolverine commented Jan 2, 2025 •

edited

Loading

superlopuh commented Jan 2, 2025

watermelonwolverine commented Jan 2, 2025

alexarice commented Jan 2, 2025

math-fehr commented Jan 2, 2025

superlopuh Jan 3, 2025

superlopuh commented Jan 3, 2025

alexarice commented Jan 3, 2025

superlopuh commented Jan 3, 2025

watermelonwolverine commented Jan 4, 2025

superlopuh commented Jan 5, 2025

superlopuh commented Jan 5, 2025

alexarice left a comment •

edited

Loading

alexarice Jan 6, 2025

alexarice Jan 6, 2025

alexarice Jan 6, 2025

superlopuh Jan 6, 2025

superlopuh Jan 6, 2025

superlopuh Jan 6, 2025

alexarice Jan 6, 2025

alexarice Jan 6, 2025

alexarice Jan 6, 2025

alexarice Jan 6, 2025

		with pytest.raises(AssertionError):
		IntegerAttr(1, i8).to_bool()



		def test_IntegerAttr___bool__():
		assert not IntegerAttr.from_bool(False)

	assert not IntegerAttr.from_bool(False)
	assert not BoolAttr.from_bool(False)

	assert IntegerAttr.from_bool(True)
	assert BoolAttr.from_bool(True)

dialects: (builtin) Fix documentation for parsing of bools, and add conversion to Python bool #3689

Are you sure you want to change the base?

dialects: (builtin) Fix documentation for parsing of bools, and add conversion to Python bool #3689

Conversation

watermelonwolverine commented Jan 2, 2025

codecov bot commented Jan 2, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

superlopuh commented Jan 2, 2025

watermelonwolverine commented Jan 2, 2025 • edited Loading

superlopuh commented Jan 2, 2025

watermelonwolverine commented Jan 2, 2025

alexarice commented Jan 2, 2025

math-fehr commented Jan 2, 2025

Choose a reason for hiding this comment

superlopuh commented Jan 3, 2025

alexarice commented Jan 3, 2025

superlopuh commented Jan 3, 2025

watermelonwolverine commented Jan 4, 2025

superlopuh commented Jan 5, 2025

superlopuh commented Jan 5, 2025

alexarice left a comment • edited Loading

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

codecov bot commented Jan 2, 2025 •

edited

Loading

watermelonwolverine commented Jan 2, 2025 •

edited

Loading

alexarice left a comment •

edited

Loading