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

PR#7315: refine some error locations #736

Merged
merged 1 commit into from Aug 4, 2016

Conversation

Projects
None yet
3 participants
@gasche
Member

gasche commented Aug 3, 2016

Fixes MPR#7315 and some related imprecise messages -- I suspect that more remain, and started building a testsuite.

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Aug 3, 2016

Contributor

Ah, I was also working on it.

trunk...alainfrisch:better_loc_unbound_constructor

I'd propose to change the interface of Typetexp.find_type (and then other Typetexp.find_* functions as well) to take a longident Location.loc instead of two separate arguments. This encourage proper use on the call site, passing the exact location corresponding to the item being looked up. This would catch, for instance, the weird location on:

# type exn += X = Y;;
Characters 12-17:
  type exn += X = Y;;
              ^^^^^
Error: Unbound constructor Y
Contributor

alainfrisch commented Aug 3, 2016

Ah, I was also working on it.

trunk...alainfrisch:better_loc_unbound_constructor

I'd propose to change the interface of Typetexp.find_type (and then other Typetexp.find_* functions as well) to take a longident Location.loc instead of two separate arguments. This encourage proper use on the call site, passing the exact location corresponding to the item being looked up. This would catch, for instance, the weird location on:

# type exn += X = Y;;
Characters 12-17:
  type exn += X = Y;;
              ^^^^^
Error: Unbound constructor Y
@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Aug 3, 2016

Member

That sounds like a good idea. Feel free to integrate my tests, add some, and send a pull-request with your implementation.

Member

gasche commented Aug 3, 2016

That sounds like a good idea. Feel free to integrate my tests, add some, and send a pull-request with your implementation.

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Aug 3, 2016

Contributor

I'm not sure I'll have time to finalize that quickly, so it's perhaps better to merge this PR and keep the API change in mind for later.

Do you think the two failing tests are related to this PR?

    tests/typing-misc/polyvars.ml
    tests/typing-objects/pr6383.ml.reference
Contributor

alainfrisch commented Aug 3, 2016

I'm not sure I'll have time to finalize that quickly, so it's perhaps better to merge this PR and keep the API change in mind for later.

Do you think the two failing tests are related to this PR?

    tests/typing-misc/polyvars.ml
    tests/typing-objects/pr6383.ml.reference
@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Aug 3, 2016

Member

Yes, the tests were related to changed location information. I fixed the tests and integrated your own changes to extension redefinitions (plus lookup of the extensible type's name). I'll wait to leave some opportunity for review, then merge in trunk. Thanks for the advice!

Member

gasche commented Aug 3, 2016

Yes, the tests were related to changed location information. I fixed the tests and integrated your own changes to extension redefinitions (plus lookup of the extensible type's name). I'll wait to leave some opportunity for review, then merge in trunk. Thanks for the advice!

Show outdated Hide outdated testsuite/tests/typing-misc/polyvars.ml
@@ -9,12 +9,12 @@ Error: This pattern matches values of type [? `A | `B ]
|}];;
let f x = ignore (match x with #ab -> 1); ignore (x : [`A]);;
[%%expect{|
Line _, characters 31-34:
Line _, characters 32-34:

This comment has been minimized.

@alainfrisch

alainfrisch Aug 3, 2016

Contributor

One could argue that the pattern here is #ab, not ab, so the old location might be better.

@alainfrisch

alainfrisch Aug 3, 2016

Contributor

One could argue that the pattern here is #ab, not ab, so the old location might be better.

Show outdated Hide outdated typing/typecore.ml
@@ -1358,7 +1358,7 @@ let rec type_pat ~constrs ~labels ~no_existentials ~mode ~explode ~env
pat_extra = extra :: p.pat_extra}
in k p)
| Ppat_type lid ->
let (path, p,ty) = build_or_pat !env loc lid.txt in
let (path, p,ty) = build_or_pat !env lid.loc lid.txt in

This comment has been minimized.

@alainfrisch

alainfrisch Aug 3, 2016

Contributor

One should probably pass both loc, the location of the pattern (for errors related to the pattern); and lid (including lid.loc) to be able to report errors related to the lookup of this identifier.

@alainfrisch

alainfrisch Aug 3, 2016

Contributor

One should probably pass both loc, the location of the pattern (for errors related to the pattern); and lid (including lid.loc) to be able to report errors related to the lookup of this identifier.

This comment has been minimized.

@gasche

gasche Aug 3, 2016

Member

Indeed, and this fixes the mistake you commented about above.

@gasche

gasche Aug 3, 2016

Member

Indeed, and this fixes the mistake you commented about above.

let f (x: #M.foo) = 0;;
^^^^^^
^^^^^

This comment has been minimized.

@gasche

gasche Aug 3, 2016

Member

Note: it's not possible to refine the location here to M only without adding more location information inside Longident.t, which would result in a painful change to the interface used by everyone manipulating the parsetree -- not a good idea.

@gasche

gasche Aug 3, 2016

Member

Note: it's not possible to refine the location here to M only without adding more location information inside Longident.t, which would result in a painful change to the interface used by everyone manipulating the parsetree -- not a good idea.

@matejkosik

This comment has been minimized.

Show comment
Hide comment
@matejkosik

matejkosik Aug 4, 2016

The original behavior:

# type foo = (unit,unit,unit,unit) bar;;
Error: Unbound type constructor bar
# type foo = unit bar;;
Error: Unbound type constructor bar

The new behavior:

# type foo = (unit,unit,unit,unit) bar;;
Error: Unbound type constructor bar
# type foo = unit bar;;
Error: Unbound type constructor bar

It is not a big thing, but nevertheless a strict improvement, in my opinion.

matejkosik commented Aug 4, 2016

The original behavior:

# type foo = (unit,unit,unit,unit) bar;;
Error: Unbound type constructor bar
# type foo = unit bar;;
Error: Unbound type constructor bar

The new behavior:

# type foo = (unit,unit,unit,unit) bar;;
Error: Unbound type constructor bar
# type foo = unit bar;;
Error: Unbound type constructor bar

It is not a big thing, but nevertheless a strict improvement, in my opinion.

@alainfrisch alainfrisch merged commit 4aef4ce into ocaml:trunk Aug 4, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

shindere added a commit to shindere/ocaml that referenced this pull request Aug 11, 2016

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

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