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

Add specific explanation for unification errors involving functions of type unit -> _ #1505

Merged
merged 3 commits into from Dec 3, 2017

Conversation

@Armael
Copy link
Member

@Armael Armael commented Dec 1, 2017

This is another small change extracted from PR #102 by @charguer . Its purpose is to provide suggestions for the common mistake of forgetting to pass () as argument to functions e.g. print_newline. The patch has also be extended to handle the symmetric situation where a value is passed instead of a thunk (and thus a fun () -> is missing).

@garrigue mentioned offline that there could be an issue in interaction with optional arguments (in which case we would disable the message in presence of functions with optional/labeled arguments) -- but I couldn't reproduce the issue. (cf the example we noted)

  let g f = f (); f ~a:1 ()
  let f ?(a=1) () = a + 1
@Octachron
Copy link
Member

@Octachron Octachron commented Dec 1, 2017

This looks like a step in the right direction. I have few minor remarks;
I do not see your potentially problematic example in your tests, would it be useless to add it?

Also, looking at the tests, I am wondering if it would not be better to group all tests in one file rather than four?

Similarly, it would be nice to write a comment explaining why the -strict-sequence flags was added either in the test themselves or in the Makefile to anticipate the possibility to set flags on a file-by-file basis after the migration to ocamltest.

On a more subjective matter, You probably forgot sounds quite assertive without a statistical study backing this claim, maybe a Did you forget or You may have forgotten would be more neutral.

@Armael Armael force-pushed the Armael:explanation-unit-fun branch 2 times, most recently from 1530624 to bb7e76e Dec 1, 2017
@Armael
Copy link
Member Author

@Armael Armael commented Dec 1, 2017

On a more subjective matter, You probably forgot sounds quite assertive without a statistical study backing this claim, maybe a Did you forget or You may have forgotten would be more neutral.

Good point, I changed the message to "Did you forget...".

I do not see your potentially problematic example in your tests, would it be useless to add it?

This was just to keep track of the offline remark by @garrigue ; here this example is actually not problematic AFAICT.

Also, looking at the tests, I am wondering if it would not be better to group all tests in one file rather than four?

Similarly, it would be nice to write a comment explaining why the -strict-sequence flags was added either in the test themselves or in the Makefile to anticipate the possibility to set flags on a file-by-file basis after the migration to ocamltest.

I merged the tests to a single file, and added such a comment.

@Armael Armael force-pushed the Armael:explanation-unit-fun branch from bb7e76e to 2a9f252 Dec 1, 2017
@Drup
Copy link
Contributor

@Drup Drup commented Dec 2, 2017

I like this change! Looking at the implementation (which is fairly straightforward) I wondered two things:

  • Shouldn't we make sure that the type "almost agree" before triggering this ? By that, I mean only trigger it when we have () -> T and T' where T unify with T'.
  • It looks fairly easy to generalize this to any one-constructor (no-argument?) type by extending is_unit to lookup the definition in the environment. I'm not sure if it's a good idea or not.
@Armael
Copy link
Member Author

@Armael Armael commented Dec 2, 2017

Shouldn't we make sure that the type "almost agree" before triggering this ? By that, I mean only trigger it when we have () -> T and T' where T unify with T'.

Is there a way of checking whether T and T' can unify, without actually unifying them? (which triggers side effects) Otherwise I'm a bit reluctant to do an unification and mutate the types at this stage, even though this is perhaps OK since we're already in failure mode.

It looks fairly easy to generalize this to any one-constructor (no-argument?) type by extending is_unit to lookup the definition in the environment. I'm not sure if it's a good idea or not.

I've never seen or used a one-constructor, no argument type that is not unit, so I'm not sure it's worth the hassle.

@Drup
Copy link
Contributor

@Drup Drup commented Dec 2, 2017

Is there a way of checking whether T and T' can unify, without actually unifying them?

Instantiate to get a copy of the type with fresh type variables, then unify.

I've never seen or used a one-constructor, no argument type that is not unit, so I'm not sure it's worth the hassle.

Pretty sure you have seen at least one: type (_,_) eq = Eq : ('a, 'a) eq. I occasionally use normal ones too.

@lpw25
Copy link
Contributor

@lpw25 lpw25 commented Dec 2, 2017

Instantiate to get a copy of the type with fresh type variables, then unify.

That would only copy the generalised parts of the type, which I'm assuming isn't the whole type here. The right way to do this is to use the backtracking support in Btype.

@Armael Armael force-pushed the Armael:explanation-unit-fun branch 2 times, most recently from 7d75eff to 4179faf Dec 2, 2017
@Armael
Copy link
Member Author

@Armael Armael commented Dec 2, 2017

I implemented @Drup first suggestion using Btype as @lpw25 suggested.

@Armael Armael force-pushed the Armael:explanation-unit-fun branch from 4179faf to 46ede41 Dec 2, 2017
@Armael
Copy link
Member Author

@Armael Armael commented Dec 2, 2017

Just added a test that exercises the unification+backtracking check.

@Armael Armael force-pushed the Armael:explanation-unit-fun branch from 46ede41 to 8db7de2 Dec 2, 2017
@gasche
gasche approved these changes Dec 3, 2017
Copy link
Member

@gasche gasche left a comment

Armaël and myself went into a rabbit hole to understand whether Unify _ was really the only kind of exception that Ctype.unify could raise; the implementation seems to also raise Unification_recursive_abbrev, but actually it looks like dead code -- and other parts of typecore also only catch Unify anyway.

With this detail out of the way, I believe the PR is correct.

@gasche gasche merged commit 7cd7644 into ocaml:trunk Dec 3, 2017
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@garrigue
Copy link
Contributor

@garrigue garrigue commented Dec 4, 2017

Ctype.unify can also raise Ctype.Tags, for conflicting polymorphic variant hashtags.
I admit very little is done to support that properly, except that you should still get some explanation in the fatal error report.

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

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.