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

more precise principality warning regarding record fields disambiguation #1661

Merged
merged 3 commits into from Mar 15, 2018

Conversation

Projects
None yet
4 participants
@trefis
Copy link
Contributor

trefis commented Mar 15, 2018

I believe warning 18 triggers wrongly on some record field disambiguation, for example in the following cases:

let c =
  let x = ({ contents = { M.lbl = 3 } } : M.r ref) in
  !x.lbl
;;
[%%expect{|
val c : int = 3
|}, Principal{|
Line _, characters 5-8:
    !x.lbl
       ^^^
Warning 18: this type-based field disambiguation is not principal.
val c : int = 3
|}]

let r arg =
  match arg with
  | (x : M.r ref) ->
    !x.lbl
;;
[%%expect{|
val r : M.r ref -> int = <fun>
|}, Principal{|
Line _, characters 7-10:
      !x.lbl
         ^^^
Warning 18: this type-based field disambiguation is not principal.
val r : M.r ref -> int = <fun>
|}]

This PR adds quite a lot of tests, mostly for things which currently behave correctly. But I hadn't noticed any other tests regarding type based disambiguation in the testsuite so I figured it couldn't hurt to add some. If I missed some preexisting one, please do point me at them and I'll trim this PR a bit.

Apart from the examples above which are fixed by this PR, there are some other examples (added to the testsuite) which are left "broken" (IMHO), e.g.

let u = function
  | ({ contents = { M.lbl = _ } } : M.r ref) as x ->
    !x.lbl
;;
[%%expect{|
val u : M.r ref -> int = <fun>
|}, Principal{|
Line _, characters 7-10:
      !x.lbl
         ^^^
Warning 18: this type-based field disambiguation is not principal.
val u : M.r ref -> int = <fun>
|}]

But these have another source, which I will soon discuss in #1655

@trefis

This comment has been minimized.

Copy link
Contributor Author

trefis commented Mar 15, 2018

Also: all the tests I have added concern record fields disambiguation.
I haven't looked at all at constructor disambiguation.

@lpw25

lpw25 approved these changes Mar 15, 2018

Copy link
Contributor

lpw25 left a comment

The change is clearly correct, and more tests seems a good idea.

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Mar 15, 2018

But I hadn't noticed any other tests regarding type based disambiguation in the testsuite so I figured it couldn't hurt to add some.

I like your style.

@trefis

This comment has been minimized.

Copy link
Contributor Author

trefis commented Mar 15, 2018

Thanks for the review!
I'll rebase, update Changes and merge now that both travis and appveyor have confirmed everything's fine.

@trefis trefis force-pushed the trefis:principler-disambiguation branch from e9c7b1d to 4f4bc2b Mar 15, 2018

@trefis trefis force-pushed the trefis:principler-disambiguation branch from 4f4bc2b to 7175587 Mar 15, 2018

@trefis trefis merged commit 92b605f into ocaml:trunk Mar 15, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@trefis trefis deleted the trefis:principler-disambiguation branch Mar 15, 2018

@garrigue

This comment has been minimized.

Copy link
Contributor

garrigue commented Mar 19, 2018

Nitpicking: I got mislead by the title of this PR (and its abstract).
I thought at first that you where improving principality analysis, and was trying to understand your example, before going the code, and seeing that this was a one-liner bug fix (and trivially correct).
Since PRs about bug fix are addressed at reviewers, better be clear about that.
(By the way, the example is precisely one that relies on minimality, and is not strictly principal)

Concerning your second example, as you state, this is just a pointer to #1655.

@trefis

This comment has been minimized.

Copy link
Contributor Author

trefis commented Mar 19, 2018

Sorry, I should have made the title more precise...

@garrigue

This comment has been minimized.

Copy link
Contributor

garrigue commented Mar 19, 2018

And I forgot to mention that adding examples to the test suite is great. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.