Skip to content

Fix10768#11340

Merged
garrigue merged 3 commits intoocaml:trunkfrom
COCTI:fix10768
Jun 22, 2022
Merged

Fix10768#11340
garrigue merged 3 commits intoocaml:trunkfrom
COCTI:fix10768

Conversation

@garrigue
Copy link
Contributor

This is an alternative fix to #10768.
Compared to #11339, it duplicates types of first class modules, the same way as in Typecore.type_cases, rather than disabling scope checking, which is potentially dangerous.

@garrigue garrigue merged commit 15ae51c into ocaml:trunk Jun 22, 2022
@garrigue
Copy link
Contributor Author

Thanks.

@Octachron
Copy link
Member

@garrigue : do you plan to cherry-pick the fix to the 5.0 branch later, or should I take care of it?

@garrigue
Copy link
Contributor Author

I'll do it just now.

garrigue added a commit that referenced this pull request Jun 22, 2022
Fix #10768: typechecking regression when combining first class modules and GADTs.
@garrigue
Copy link
Contributor Author

To cherry-pick, I had to adapt the results to not having #10364.
Which lets me wonder whether it might be better to have #10364 in 5.0.
This is a kind of bug fix, but it types less programs rather than more...

@Octachron
Copy link
Member

That sounds reasonable to me, #10364 is a rather old bug fix that was frozen during multicore integration. It makes sense to integrate it before the first alpha but let's ask for @trefis's opinion as the reviewer.

However, looking at the testsuite changes for #10364, I only see functions with improved error messages, one function that now type in non-principal mode, and two functions that type in principal mode but with a warning. Do you have an example of a program that no longer typecheck?

@garrigue
Copy link
Contributor Author

You're right. The principal mode is stricter, but only for warnings, so this makes sense to cherry-pick it.

dra27 added a commit to dra27/ocaml that referenced this pull request Sep 27, 2023
…request PR#12198 from shindere/merge-debugger-makefile

Merge debugger/Makefile into the root Makefile
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants