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

Fix bug on unboxed records with one polymorphic field #1495

Merged
merged 1 commit into from Nov 30, 2017

Conversation

Projects
None yet
3 participants
@alainfrisch
Copy link
Contributor

alainfrisch commented Nov 29, 2017

Fixes MPR#7682.

@alainfrisch alainfrisch force-pushed the fix_7682 branch from 4cb6c0f to 8136c88 Nov 29, 2017

@gasche

gasche approved these changes Nov 29, 2017

Copy link
Member

gasche left a comment

I believe that the fix is correct.

I find it a bit frustrating that we have so little guidance from the type system on the various preconditions/postconditions/invariants expected by various functions of the type checker. This issue would have been caught much earlier if we distinguished polymorphic type schemes from monomorphic types, for example, or had some way to describe the subclass of types that scrape returns.

Another fix (I believe) would be to change Typeopt.classify to call itself recursively in the Tpoly case instead of forbidding this case. I have the gut feeling that also applying this (redundant) fix would make the code safer in the future.

(I won't merge this PR immediately to give @alainfrisch a chance to give his opinion on the suggested second change.)

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Nov 29, 2017

(There is something strange with the way get_unboxed_type_representaion works recursively. Can it traverse several type definitions at once?)

@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

alainfrisch commented Nov 29, 2017

Another fix (I believe) would be to change Typeopt.classify to call itself recursively in the Tpoly case instead of forbidding this case.

Indeed. But other uses of Typedecl.get_unboxed_type_representation might also benefit from the change in the PR (check_unboxed_gadt_arg already traverses Tpoly explicitly; but compute_immediacy doesn't; plus other uses of scrape in Typeopt).

I have the gut feeling that also applying this (redundant) fix would make the code safer in the future.

An even more "defensive" approach would be to return Any instead of the assert false in classify. I'm not a big fan of such defensive style, because it can hide the root cause of the problem, whose proper fix would benefit other places.

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Nov 29, 2017

I agree that the change you did is needed. But I also think that having classify recurse through Tpoly nodes is always the right thing to do (even if change the invariants/specification of get_unboxed_... later), so it could be done as well.

@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

alainfrisch commented Nov 29, 2017

But I also think that having classify recurse through Tpoly nodes is always the right thing to do

I'm not fond of adding code known to be currently dead. Moreover, if the change you suggest was applied, it would have avoided the current bug but masked other (less serious) problems.

@alainfrisch alainfrisch added this to the 4.07 milestone Nov 29, 2017

@gasche gasche merged commit 282d8c4 into trunk Nov 30, 2017

4 checks passed

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

This comment has been minimized.

Copy link
Member

gasche commented Nov 30, 2017

We don't agree on this detail point but I merged anyway. Thanks for the patch!

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Nov 30, 2017

Should we have this in the 4.06 maintenance branch? Personally I am sure that the change is safe and fixes a bug.

@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

alainfrisch commented Dec 1, 2017

Gabriel merged before letting me adapt the Changes file to mention him; fixed in aa7dd54.

AFAICT, the bug cannot lead to miscompilation or accepting ill-typed program, and is not a regression. I wouldn't mind including it in 4.06.1, but I wouldn't particularly push for inclusion either.

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Dec 1, 2017

I had a chat with Xavier Leroy yesterday evening about what we want in 4.06, and we came to the same conclusion as you above: only critical bugs with obviously-correct fixes should be considered, and the fact that this ones fails at compile-time instead of producing wrong code (or accepting wrong programs) makes it non-critical.

@disteph

This comment has been minimized.

Copy link

disteph commented Dec 5, 2017

Thank you both for your quick response!
I understand the principles of non-critical bug fixes.
Does this mean that I can get the fix by switching to 4.07.0+trunk, with the risk of not being able to use opam packages that won't get themselves compatible with 4.07.0 (or even declare themselves to be) until some time in May? :-)

@alainfrisch alainfrisch deleted the fix_7682 branch Jul 2, 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.