Fix unintentional drift introduced in typing tests#33
Merged
mshinwell merged 6 commits intomshinwell:5.2-runtime-wip-mainfrom Aug 21, 2024
Merged
Conversation
Author
|
@ccasin Could you please review this? |
cfca4b0 to
a524c8c
Compare
|
The commits here all look fine. I also recreated Nick's methodology and found a few files that are debatably typing tests and need more attention - Nick and I discussed and he'll follow up in a second PR if needed. For the tests that are definitely about typing, I only spotted one remaining minor issue (a duplicated |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix issues with typing tests introduced in the 5.2 merge.
My methodology for obtaining the list of possibly-problematic tests:
Misc.Style.inline_codeto stop inserting quotation marks. This removes noise between upstream and internal versions of tests. (The styling change to start usinginline_codein a more widespread way is in 5.2.)I then classified the test diffs into these categories:
The remaining cases had actual problems. This PR fixes all problems I identified.
Problem #1: "Error: Don't know how to untag this type. Only int can be untagged."
Instances:
The merge introduced a new assumption from upstream (#12375) that a type that
Typeoptthinks is immediate must have its lowest bit set to 1. But, internally,Typeoptalso thinks of unboxed types likefloat#andint#as immediate. This is strange, and allowed programs like these to pass type-checking:@ccasin and I decided to check whether the sort of the type in question is
Valueas an additional criteria for@untagged.Problem #2: Extra diff
There were a few issues where chunks of code were duplicated. I deleted the duped tests. See the commit messages for why.
Instances:
Problem #3: Wrong location in expect tests
A failure to map over all locations in
ast_mapper.mlmeant that a location was wrong inocaml/testsuite/tests/typing-layouts/error_message_attr.ml.