Skip to content

Fix unintentional drift introduced in typing tests#33

Merged
mshinwell merged 6 commits intomshinwell:5.2-runtime-wip-mainfrom
ncik-roberts:5.2-runtime-wip-main-typing-test-fixes
Aug 21, 2024
Merged

Fix unintentional drift introduced in typing tests#33
mshinwell merged 6 commits intomshinwell:5.2-runtime-wip-mainfrom
ncik-roberts:5.2-runtime-wip-main-typing-test-fixes

Conversation

@ncik-roberts
Copy link
Copy Markdown

Fix issues with typing tests introduced in the 5.2 merge.

My methodology for obtaining the list of possibly-problematic tests:

  • Change Misc.Style.inline_code to stop inserting quotation marks. This removes noise between upstream and internal versions of tests. (The styling change to start using inline_code in a more widespread way is in 5.2.)
  • See which tests have a diff between the merge base b532dff (in flambda-backend) and a workspace with the previous change applied.

I then classified the test diffs into these categories:

  • Improvements pulled in from upstream
  • Test files that are identical between the tip of the 5.2 merge and upstream main
  • Changes that aren’t actually changes: the merge base already quoted some inline code, so my methodology found some false positives.

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:

ocaml/testsuite/tests/typing-layouts-bits32/basics.ml
ocaml/testsuite/tests/typing-layouts-bits32/basics_alpha.ml
ocaml/testsuite/tests/typing-layouts-bits64/basics.ml
ocaml/testsuite/tests/typing-layouts-bits64/basics_alpha.ml
ocaml/testsuite/tests/typing-layouts-float32/basics.ml
ocaml/testsuite/tests/typing-layouts-float64/basics.ml
ocaml/testsuite/tests/typing-layouts-word/basics.ml
ocaml/testsuite/tests/typing-layouts-word/basics_alpha.ml
ocaml/testsuite/tests/typing-layouts/erasable_annot.ml

The merge introduced a new assumption from upstream (#12375) that a type that Typeopt thinks is immediate must have its lowest bit set to 1. But, internally, Typeopt also thinks of unboxed types like float# and int# as immediate. This is strange, and allowed programs like these to pass type-checking:

external blah : (float# [@untagged]) -> unit = "..."

@ccasin and I decided to check whether the sort of the type in question is Value as 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:

ocaml/testsuite/tests/typing-local/local.ml
ocaml/testsuite/tests/typing-modes/portable-contend.ml
ocaml/testsuite/tests/typing-layouts-float64/parsing.ml

Problem #3: Wrong location in expect tests

A failure to map over all locations in ast_mapper.ml meant that a location was wrong in ocaml/testsuite/tests/typing-layouts/error_message_attr.ml.

@ncik-roberts
Copy link
Copy Markdown
Author

@ccasin Could you please review this?

@ncik-roberts ncik-roberts force-pushed the 5.2-runtime-wip-main-typing-test-fixes branch from cfca4b0 to a524c8c Compare August 20, 2024 21:14
@mshinwell mshinwell merged commit 398a786 into mshinwell:5.2-runtime-wip-main Aug 21, 2024
@ccasin
Copy link
Copy Markdown

ccasin commented Aug 21, 2024

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 expect in typecore_errors.ml) and will make a PR to fix that momentarily.

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