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

remove unreachable error variant: Make_seltype_nongen #1746

Merged
merged 2 commits into from May 5, 2018

Conversation

Projects
None yet
5 participants
@Octachron
Copy link
Contributor

commented Apr 30, 2018

This PR removes from the type checker an unreachable error variant: Make_seltype_nongen which is supposed to raise the following error message

Self type should not occur in the non-generic type

Currently, this error can only be emitted if Typecore.type_let or Typecore.type_expr raise an uncaught Unify exception. As far as I can see, this behavior is no longer possible; and it would create issues in Typemod which calls those two functions indirectly.

(Originally, those try ... with ... might have been added 20 years ago to caught the exceptions raised by make_nongen, which is no longer present in the compiler.)

@Drup

Drup approved these changes Apr 30, 2018

Copy link
Contributor

left a comment

We already discussed this, and as I said then, I'm pretty convinced you are right and this piece of code is indeed unused.

My personal hypothesis is that this particular error was made impossible by a change in syntax/scoping for the self variable and was then dead for 10+ years. I'm curious what @garrigue will say. :)

@garrigue

This comment has been minimized.

Copy link
Contributor

commented May 4, 2018

I don't remember. This looks like a dirty hack anyway.
Maybe Jerome Vouillon remembers, but it's so old.

@Octachron

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2018

@garrigue , are you fine with removing this dirty and long-forgotten hack?

@trefis

This comment has been minimized.

Copy link
Contributor

commented May 4, 2018

If I'm not mistaken Jérôme's github username is @vouillon , and hopefully he will be notified because of this comment.

I'd personally be tempted to merge this: the worst that can happen is that someone trying to compile their code will get an uncaught Unify exception instead of a proper error, that person will complain on mantis and then we'll have a test for this. (But I think I already proposed something similar on another PR in the past and @gasche wasn't too happy about the approach)

@Drup

This comment has been minimized.

Copy link
Contributor

commented May 4, 2018

Well, It's not like Self type should not occur in the non-generic type t is much clearer than Uncaught exception: Unify _ ....

@gasche

This comment has been minimized.

Copy link
Member

commented May 4, 2018

Indeed in general I'm not fond of "if we are not sure, let's fire and wait for users to report bug". But in this case it looks like @Octachron is certain that this error is now dead code, and even has a reasonable historical explanation of what happened. So I would support merging this.

@gasche

gasche approved these changes May 4, 2018

@Octachron Octachron force-pushed the Octachron:rip_seltype_nongen branch from 4a720a1 to 01ff3fb May 4, 2018

@gasche gasche merged commit 9b10a77 into ocaml:trunk May 5, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.