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

MPR#7937: avoid Unify exception when looking for type declaration #2287

Merged
merged 1 commit into from Mar 7, 2019

Conversation

@Octachron
Copy link
Contributor

commented Mar 5, 2019

MPR#7937:
This PR modifies Ctype.extract_concrete_typedecl in order to never raise the Unify exception. This function is notably used when typing constructor and record literals in presence of disambiguation (since 4.01).

However, the current uses of this function do not take in account the pathologic cases where extract_concrete_typedecl raises an Unify _ error. For instance,

type 'a t = [< `A of int & 'a ] as 'a
let f: 'a. 'a t -> _ = function true -> ()

lead to the uncaught Unify exception reported in MPR#7937.

This PR proposes to fix this uncaught error by unifying the Unify _ exception case with the Not_found exception case, which is raised when no concrete type declarations were found.

@garrigue

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

I was going to look into it, but you did beat me on this one.
The cause of the unification error here is a bit complex, but I agree that this should never happen if the type at hand is really a concrete datatype, so it is correct to replace try_expand_once by try_expand_safe (which was precisely introduced for this kind of uses).

You just need to add a Changes entry.

@Octachron Octachron force-pushed the Octachron:safe_extract_concrete branch from 30219fb to 5b1b65d Mar 6, 2019
@Octachron

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

I added a Change entry in trunk rather than 4.08, since the issue has been here for 7 versions.

@garrigue

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

Thank you.

@garrigue garrigue merged commit 00c18be into ocaml:trunk Mar 7, 2019
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.