core: Rename `typ` properties to `type` #1240

kingiler · 2023-07-06T11:24:30Z

Address #1174

review-notebook-app · 2023-07-06T11:24:34Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

codecov · 2023-07-06T11:28:09Z

Codecov Report

Patch coverage: 82.02% and no project coverage change.

Comparison is base (db4d5b3) 88.93% compared to head (e1dc929) 88.94%.

❗ Current head e1dc929 differs from pull request most recent head c03451b. Consider uploading reports for the commit c03451b to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1240   +/-   ##
=======================================
  Coverage   88.93%   88.94%           
=======================================
  Files         175      175           
  Lines       23642    23590   -52     
  Branches     3587     3568   -19     
=======================================
- Hits        21026    20981   -45     
+ Misses       2044     2042    -2     
+ Partials      572      567    -5

Impacted Files	Coverage Δ
xdsl/dialects/cmath.py	`75.75% <0.00%> (ø)`
xdsl/dialects/experimental/math.py	`73.61% <0.00%> (ø)`
xdsl/interpreters/experimental/pdl.py	`73.25% <0.00%> (ø)`
...dsl/transforms/experimental/Apply1DMPIToStencil.py	`0.00% <0.00%> (ø)`
xdsl/transforms/experimental/dmp/scatter_gather.py	`30.86% <0.00%> (ø)`
...sforms/experimental/dmp/stencil_global_to_local.py	`25.96% <0.00%> (ø)`
xdsl/transforms/lower_snitch_runtime.py	`95.74% <ø> (ø)`
docs/Toy/toy/dialects/toy.py	`68.12% <22.50%> (ø)`
xdsl/dialects/affine.py	`76.82% <33.33%> (ø)`
xdsl/dialects/printf.py	`91.66% <50.00%> (ø)`
... and 47 more

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

docs/Toy/toy/dialects/toy.py

AntonLydike · 2023-07-06T14:01:49Z

I'm curious why this PR also re-orders all imports, are you running a custom formatting script?

It would help immensely with reviewing if we only had relevant changes in the diff!

math-fehr · 2023-07-06T15:22:39Z

I'll look at it once the imports changes are removed, but otherwise it looks good to me!

kingiler · 2023-07-06T15:57:46Z

The import changes issue is addressed in #1243. Now the only changes should be the apis.

webmiche · 2023-07-06T16:14:07Z

Let me repeat my recent comment on the issue here:

Why do we want to do this? type is a keyword right? Feels better to stay with typ, IMO 🤔

adutilleul

If we want to be consistent, you should also rename all the stray variable names containing typ or typs, not sure I catched all of them in my review but you should take a look at this.

adutilleul · 2023-07-06T16:11:07Z

docs/Toy/toy/dialects/toy.py

@@ -230,7 +230,7 @@ def verify_(self):
            raise VerifyException("Expected last op of FuncOp to be a ReturnOp")

        operand = last_op.input
-        operand_typ = None if operand is None else operand.typ
+        operand_typ = None if operand is None else operand.type


should be named operand_type here

adutilleul · 2023-07-06T16:11:35Z

tests/dialects/test_llvm.py

@@ -86,7 +86,7 @@ def test_llvm_getelementptr_op_invalid_construction():
 def test_llvm_getelementptr_op():
    size = arith.Constant.from_int_and_width(1, 32)
    ptr = llvm.AllocaOp.get(size, builtin.i32)
-    ptr_typ = llvm.LLVMPointerType.typed(ptr.res.typ)
+    ptr_typ = llvm.LLVMPointerType.typed(ptr.res.type)


should be named ptr_type here

adutilleul · 2023-07-06T16:12:01Z

xdsl/dialects/gpu.py

-        assert isinstance(self.result.typ, memref.MemRefType)
-        typ: memref.MemRefType[Attribute] = self.result.typ
+        assert isinstance(self.result.type, memref.MemRefType)
+        typ: memref.MemRefType[Attribute] = self.result.type
        ndyn_typ = len([i for i in typ.shape.data if i.value.data == -1])


should be named ndyn_type here

adutilleul · 2023-07-06T16:12:35Z

docs/Toy/toy/dialects/toy.py

@@ -230,7 +230,7 @@ def verify_(self):
            raise VerifyException("Expected last op of FuncOp to be a ReturnOp")

        operand = last_op.input
-        operand_typ = None if operand is None else operand.typ
+        operand_typ = None if operand is None else operand.type

        return_typs = self.function_type.outputs.data


should be named return_types here

adutilleul · 2023-07-06T16:27:01Z

Let me repeat my recent comment on the issue here:

Why do we want to do this? type is a keyword right? Feels better to stay with typ, IMO thinking

type is a built-in function, so you could prefix __builtin__ (builtin.type) to still use it but that might be confusing indeed

math-fehr · 2023-07-06T23:41:11Z

Let me repeat my recent comment on the issue here:

Why do we want to do this? type is a keyword right? Feels better to stay with typ, IMO 🤔

type is not a keyword, right? It's a builtin function though. I think it should be safe to have type as a field, as it anyway needs self.type to access it.

I personally think that we should use type rather than typ for fields, which is the case here. What do you think?

webmiche · 2023-07-07T07:05:46Z

Let me repeat my recent comment on the issue here:

Why do we want to do this? type is a keyword right? Feels better to stay with typ, IMO thinking

type is not a keyword, right? It's a builtin function though. I think it should be safe to have type as a field, as it anyway needs self.type to access it.

I personally think that we should use type rather than typ for fields, which is the case here. What do you think?

Okay, sure. I think my point still stands though. I feel that we will run into cases where this will get annoying, so I would try to circumvent it and do typ from the get go. But I don't have a strong opinion on it. If you feel type is the way, I am happy to take type.

kingiler · 2023-07-07T10:27:43Z

I am thinking about the scope of this PR. I think having a class field named type is fine. But for the others variables named typ, should they put into next PR?

math-fehr · 2023-07-07T12:42:46Z

I am thinking about the scope of this PR. I think having a class field named type is fine. But for the others variables named typ, should they put into next PR?

I think that's exacly @michel-steuwer point here. I believe class fields should be named type, as they don't shadow anything. However, variables named typ might be a bit dangerous, and that's where I believe Michel wants to avoid them. I'm fine with both for the variables, as anyway we have pyright telling us that we are doing something wrong.

kingiler · 2023-07-07T12:47:48Z

I can do a git grep "typ[^ei]" and rename those variables with a proper name.

superlopuh

I closed the issue before seeing this PR, and I actually think that the diff is a strict improvement. I don't feel very strongly about this, but I think we should merge this PR.

This is a follow up to a previous PR #1240 .

kingiler requested review from meshtag, AntonLydike, PapyChacal, math-fehr, webmiche, superlopuh, compor, nazavode and georgebisbas as code owners July 6, 2023 11:24

kingiler changed the title ~~Rename typ properties to type~~ Rename typ properties to type Jul 6, 2023

compor reviewed Jul 6, 2023

View reviewed changes

docs/Toy/toy/dialects/toy.py Outdated Show resolved Hide resolved

AntonLydike added the API Related to changes regarding API of constructs label Jul 6, 2023

kingiler changed the title ~~Rename typ properties to type~~ core: Rename typ properties to type Jul 6, 2023

kingiler requested a review from compor July 6, 2023 15:58

adutilleul requested changes Jul 6, 2023

View reviewed changes

kingiler added the core xDSL core (ir, textual format, ...) label Jul 6, 2023

Rename properties to

e1dc929

Merge branch 'main' into rename_typ

d840362

Rebase from main and update code

c03451b

superlopuh approved these changes Jul 7, 2023

View reviewed changes

kingiler requested a review from adutilleul July 7, 2023 13:52

adutilleul approved these changes Jul 7, 2023

View reviewed changes

Merge branch 'main' into rename_typ

cde5ff4

kingiler merged commit adfc0ef into xdslproject:main Jul 7, 2023
9 checks passed

kingiler deleted the rename_typ branch July 7, 2023 14:02

kingiler mentioned this pull request Jul 10, 2023

misc: Rename Typ to Type #1260

Merged

kingiler added a commit that referenced this pull request Jul 12, 2023

misc: Rename Typ to Type (#1260)

a6c78eb

This is a follow up to a previous PR #1240 .

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: Rename `typ` properties to `type` #1240

core: Rename `typ` properties to `type` #1240

kingiler commented Jul 6, 2023 •

edited

review-notebook-app bot commented Jul 6, 2023

codecov bot commented Jul 6, 2023 •

edited

AntonLydike commented Jul 6, 2023

math-fehr commented Jul 6, 2023

kingiler commented Jul 6, 2023 •

edited

webmiche commented Jul 6, 2023

adutilleul left a comment

adutilleul Jul 6, 2023

adutilleul Jul 6, 2023

adutilleul Jul 6, 2023

adutilleul Jul 6, 2023 •

edited

adutilleul commented Jul 6, 2023

math-fehr commented Jul 6, 2023

webmiche commented Jul 7, 2023

kingiler commented Jul 7, 2023

math-fehr commented Jul 7, 2023

kingiler commented Jul 7, 2023

superlopuh left a comment

core: Rename typ properties to type #1240

core: Rename typ properties to type #1240

Conversation

kingiler commented Jul 6, 2023 • edited

review-notebook-app bot commented Jul 6, 2023

codecov bot commented Jul 6, 2023 • edited

Codecov Report

AntonLydike commented Jul 6, 2023

math-fehr commented Jul 6, 2023

kingiler commented Jul 6, 2023 • edited

webmiche commented Jul 6, 2023

adutilleul left a comment

Choose a reason for hiding this comment

adutilleul Jul 6, 2023

Choose a reason for hiding this comment

adutilleul Jul 6, 2023

Choose a reason for hiding this comment

adutilleul Jul 6, 2023

Choose a reason for hiding this comment

adutilleul Jul 6, 2023 • edited

Choose a reason for hiding this comment

adutilleul commented Jul 6, 2023

math-fehr commented Jul 6, 2023

webmiche commented Jul 7, 2023

kingiler commented Jul 7, 2023

math-fehr commented Jul 7, 2023

kingiler commented Jul 7, 2023

superlopuh left a comment

Choose a reason for hiding this comment

core: Rename `typ` properties to `type` #1240

core: Rename `typ` properties to `type` #1240

kingiler commented Jul 6, 2023 •

edited

codecov bot commented Jul 6, 2023 •

edited

kingiler commented Jul 6, 2023 •

edited

adutilleul Jul 6, 2023 •

edited