New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Traits in PIL #1450

Closed

gzanitti wants to merge 46 commits into powdr-labs:main from gzanitti:traits_in_pil

Collaborator

gzanitti commented Jun 11, 2024

Impl of #1286.

gzanitti added 3 commits

June 11, 2024 18:41

WIP

2c5d4c8


          Impl display

81294e2


          Still WIP. Parser working, but still need to confirm some decisions.

9281efe

chriseth reviewed

View reviewed changes

analysis/src/machine_check.rs Outdated Show resolved Hide resolved

chriseth reviewed

View reviewed changes

ast/src/parsed/display.rs Outdated Show resolved Hide resolved

chriseth reviewed

View reviewed changes

ast/src/parsed/mod.rs Outdated Show resolved Hide resolved

chriseth reviewed

View reviewed changes

ast/src/parsed/mod.rs Outdated Show resolved Hide resolved

chriseth reviewed

View reviewed changes

ast/src/parsed/mod.rs Outdated Show resolved Hide resolved

chriseth reviewed

View reviewed changes

ast/src/parsed/mod.rs Outdated Show resolved Hide resolved

chriseth reviewed

View reviewed changes

importer/src/path_canonicalizer.rs Outdated Show resolved Hide resolved

chriseth reviewed

View reviewed changes

parser/src/lib.rs Show resolved Hide resolved

chriseth reviewed

View reviewed changes

parser/src/lib.rs Show resolved Hide resolved

chriseth reviewed

View reviewed changes

parser/src/powdr.lalrpop Outdated Show resolved Hide resolved

chriseth reviewed

View reviewed changes

parser/src/powdr.lalrpop Outdated Show resolved Hide resolved

gzanitti added 14 commits

June 13, 2024 09:04


          minor fix

f2c1473


          Improvements: Parser, AST, etc

1c54800


          More progress in PIL analyzer

7e90824


          More progress

362d287


          More progress in symbol definition but still lots of TODOs

2116b8f


          TraitDeclaration in pil-analyzer

9f291e5


          More progress: Separation between Impl/Trait to avoid symbols name co…

f3a3123

…lission


          Impl included in PIL-analyzer. Next step: working on substitutions

5051bac


          Main merged: conflict solved

68d62ad


          Display test fixed

3c79a40


          catching undefined traits

1df3065


          Hacky code but good progress. Need to replace vars in the impl.

5fbc748


          Basic idea working. More tests, reviews, etc are needed

2a6a5d5


          Basic idea working. More tests, reviews, etc are needed

d82405c

chriseth reviewed

View reviewed changes

ast/src/analyzed/display.rs

+                          });
+                      let s = format!(
+                          "impl{type_vars} {trait_name}{trait_vars} {{\n{methods}}}",

Member

chriseth Jun 18, 2024

There is the function format_type_scheme_around_name that could help here.

Collaborator Author

gzanitti Jun 18, 2024

I don't think it will work, because trait_vars is a vector (and type_scheme has only one element).

This difference is the result of this separation between TypeScheme and TraitScheme:

powdr/ast/src/parsed/types.rs

Line 355 in e904efd

pub struct TraitScheme<E = u64> {

Both could be unified if I model the current TypeScheme with a single element vector (ty), but I felt it would add a lot of code changes. What do you think?


          Some TODO removed

e904efd

gzanitti added 18 commits

June 25, 2024 18:38


          Issue with more than one impl

1ca69d0


          WIP. Solving conflict when more than one impl

d378daf


          Colission between impls solved. Code needs to be cleaned and improved

97e2322


          Minor fix

a9806b1


          linter

865353c


          Code improvements

b437575


          Unnecesary clone

2c28d86


          SymbolPath

8d54f14


          More tests


          Temporal solution to path issues

3162c54


          Linter, always the linter

09e6cd8


          Error message corrected

55cfb28


          Error message corrected

ce01e34


          More tests

a71c1f8


          Corrected output

50dede7


          Better error reporting

1f33244


          Some Docs

f1a24a2


          Canonicalizer

23c1cc5

gzanitti marked this pull request as ready for review

June 28, 2024 01:08

gzanitti added 2 commits

June 27, 2024 22:34


          Test fixed

48cd334


          Test fixed

ee06163

gzanitti changed the title ~~[WIP] Traits in PIL~~ Traits in PIL

Member

chriseth commented Jun 28, 2024

I see that you are starting to work on the type checking part. I think this PR is already too big to be properly reviewed at the current stage. Could you maybe split out just the parsing of the trait declarations (not even the impls), so that we can get that merged already?

This was referenced Jun 28, 2024

Trait parsing #1489

Merged

Impl parsing #1490

Merged

github-merge-queue bot pushed a commit that referenced this pull request


          Trait parsing (#1489)

70e4f7c

This PR splits from the main Trait implementation PR #1450 to simplify
the review process.

It includes only the parsing of the traits (not impls) and some
functionality necessary for the code to compile.

---------

Co-authored-by: chriseth <chris@ethereum.org>

gzanitti marked this pull request as draft

July 9, 2024 19:22

github-merge-queue bot pushed a commit that referenced this pull request


          Impl parsing (#1490)

2e7c473

This PR splits from the main Trait implementation PR
#1450 to simplify the review
process.

It includes only the parsing of the impls (trait parsing PR here: #1489
) and some functionality necessary for the code to compile.

---------

Co-authored-by: chriseth <chris@ethereum.org>

gzanitti closed this

gzanitti deleted the traits_in_pil branch

August 26, 2024 21:27

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

Labels

None yet