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

disambiguation: improve the interface of NameChoice.disambiguate #9196

Merged
merged 1 commit into from
Mar 9, 2020

Conversation

gasche
Copy link
Member

@gasche gasche commented Dec 18, 2019

This PR sits on top of #9182, and suggests a difference interface for disambiguate. The idea is that before we had lbls and scope and we had to explain the relation between the two, while now we have just scope and a filtering criterion.

scope is passed in "delayed error" style, this has not changed and remains a source of subtlety in the disambiguate implementation, but I think that this version makes the subtlety a bit clearer.

(cc @garrigue @lpw25 @trefis)

@trefis
Copy link
Contributor

trefis commented Dec 18, 2019

If I'm not mistaken, one should only look at the last commit. I had just started working on a similar patch, and while I haven't carefully reviewed yet: I like the change you propose.

typing/typecore.ml Outdated Show resolved Hide resolved
typing/typecore.ml Outdated Show resolved Hide resolved
typing/typecore.ml Outdated Show resolved Hide resolved
@gasche gasche force-pushed the clarify-disambiguation-3 branch 5 times, most recently from 9551751 to 73ced32 Compare December 19, 2019 12:25
@gasche gasche force-pushed the clarify-disambiguation-3 branch 2 times, most recently from 8cdb1ed to 9155c99 Compare February 25, 2020 11:13
@gasche
Copy link
Member Author

gasche commented Mar 6, 2020

I fixed the test output (a new warning is emitted with this implementation, which is correct) and rebased the PR against the current trunk.

typing/typecore.ml Outdated Show resolved Hide resolved
typing/typecore.ml Outdated Show resolved Hide resolved
Env.lookup_error loc' env' err
| Ok lbls ->
match filter (force candidates_in_scope) with
| Ok lbls | Error lbls ->
Copy link
Member

@Octachron Octachron Mar 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a hidden bugfix here compared to trunk. Previously, lbls could be empty here, for instance

module M = struct
  type t = { x:int; y:int}
end
type u = { a:int }
let _ = ( { M.x=0 } : u )

which triggers an assert false in Printtyp.
With this PR this is not the case anymore because we don't fold the filter error case into Ok [ ] .

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this example to the testsuite.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: I cannot reproduce the printing failure with 4.10, this is trunk-only. I wonder if it is a result of the previous refactoring PRs for NameChoice.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bug is present in 4.10, but it requires -principal and a non-principal expected type:

module M = struct
  type t = { x:int; y:int}
end
type u = { a:int }
let f (x:u) = x = { M.x=0 }

@gasche gasche force-pushed the clarify-disambiguation-3 branch 4 times, most recently from 4e70fec to bf6c8d4 Compare March 7, 2020 13:30
Copy link
Member

@Octachron Octachron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The refactoring looks good to me, as illustrated by the missing warning and printing failure that it fixes incidentally.

@gasche gasche merged commit 0fff7a4 into ocaml:trunk Mar 9, 2020
@gasche
Copy link
Member Author

gasche commented Mar 9, 2020

Thanks! Merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants