Skip to content
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.

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

Type inference error with Parser.Advanced #696

Closed
bengolds opened this issue May 23, 2022 · 4 comments · Fixed by #835
Closed

Type inference error with Parser.Advanced #696

bengolds opened this issue May 23, 2022 · 4 comments · Fixed by #835
Labels
bug Something isn't working type inference Related to type inference

Comments

@bengolds
Copy link
Contributor

There seems to be an issue with type inference when we use Parser.Advanced alongside Parser. I've attached a pretty minimal repro; I can't quite get it more minimal than this yet :)

CleanShot 2022-05-23 at 13 20 12@2x

Expected Behavior

The code compiles without issues with elm make, so I'd expect no errors in the LS.

Current Behavior

This currently throws an error in the elm language server.

Steps to Reproduce (for bugs)

elm-ls-parse.zip

Context

This issue is actually creating ~80 errors for our main project 😱

Your Environment

Elm LS 2.3.0, in both Vim and VSCode.

@razzeee
Copy link
Member

razzeee commented May 23, 2022

FYI @jmbockhorst

@jmbockhorst jmbockhorst added bug Something isn't working type inference Related to type inference labels Jun 30, 2022
@bengolds
Copy link
Contributor Author

I've finally dug into this a little bit, and found the cause of the error: the operatorCache only indexes operators by name.

However, the Parser and Parser.Advanced packages both define a |= operator with different types. So what happens is:

  1. Parser is read first, and Parser.|= gets loaded into the operator cache.
  2. When Parser.Advanced.|= is read, its definition doesn't get loaded into the operator cache since |= is already in there.
  3. When another module uses Parser.Advanced.|=, the typechecker looks up |= in the operator cache, and sees that it exists -- and points to Parser.|=!

So, there are two potential fixes that come to mind for this:
A. Just disable the operator cache. Not sure how damaging this would be for performance.
B. Have the operator cache keyed by operator name AND source file -- ie, every source file has its own operator cache. Do we have existing good data structures for this?

@razzeee
Copy link
Member

razzeee commented Nov 14, 2022

I think usually we would key by module name + operator name. But jon is the expert here.

@jmbockhorst
Copy link
Member

We should be able to get the operator import in the current source file from IImports, which would ensure it is actually imported. Then the we can build the cache with the module prefix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working type inference Related to type inference
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants