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

Better error messages for extension constructor type mismatches #1976

Merged
merged 3 commits into from Aug 9, 2018

Conversation

Projects
None yet
2 participants
@trefis
Copy link
Contributor

commented Aug 8, 2018

A bootstrap was required because all the idents are shifted because the code was previously creating a fresh ident with name "" at a frequent rate.

@gasche

This comment has been minimized.

Copy link
Member

commented Aug 8, 2018

I think the current bootstrap policy is that the bootstrap changes should come on their own?

@gasche
Copy link
Member

left a comment

The change looks correct to me, but see minor comments.

@@ -137,10 +137,11 @@ Error: Signature mismatch:
sig type t += E of int end
is not included in
sig type t += E end
Extension declarations do not match:

This comment has been minimized.

Copy link
@gasche

gasche Aug 8, 2018

Member

This seems to be a regression.

This comment has been minimized.

Copy link
@trefis

trefis Aug 8, 2018

Author Contributor

Indeed, typo on my part. I'll push a fix now.

if compare_constructor_arguments ~loc env (Ident.create "")
if
not (Ctype.equal env true (ty1 :: ext1.ext_type_params)
(ty2 :: ext2.ext_type_params))

This comment has been minimized.

Copy link
@gasche

gasche Aug 8, 2018

Member

The previous alignment of 1 and 2 parameters was nicer to read.

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2018

I think the current bootstrap policy is that the bootstrap changes should come on their own?

I thought the policy was that each commit should compile.
If I put the bootstrap on its own then the compiler doesn't compile.

@trefis trefis force-pushed the trefis:extension-mismatch branch from 08ca8b2 to d11f836 Aug 8, 2018

@gasche

This comment has been minimized.

Copy link
Member

commented Aug 9, 2018

Checking e42a2e4e8ed0c7b5ccf5f05083a9c53f803bc33b: testsuite/tests/typing-modules/Test.ml
./testsuite/tests/typing-modules/Test.ml:180.81: [long-line] line is over 80 columns

You need to break a line in

 module M : sig type t += E of { x : int } end = struct type t += E of int end;;
@gasche

gasche approved these changes Aug 9, 2018

Copy link
Member

left a comment

Good to merge when CI passes.

(I still think that the bootstrap commit should be separate, but I won't fight for it.)

@trefis trefis force-pushed the trefis:extension-mismatch branch from d11f836 to 327c078 Aug 9, 2018

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2018

Raah, I really need to setup up that precommit hook...
Anyway, I amended the commit to fix the issue and pushed again.

@trefis trefis force-pushed the trefis:extension-mismatch branch from 327c078 to c140db6 Aug 9, 2018

@trefis trefis merged commit 7b44242 into ocaml:trunk Aug 9, 2018

2 checks passed

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

@trefis trefis deleted the trefis:extension-mismatch branch Aug 9, 2018

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.