Skip to content

Enable error baselines #425

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

Merged
merged 12 commits into from
Mar 10, 2025
Merged

Enable error baselines #425

merged 12 commits into from
Mar 10, 2025

Conversation

jakebailey
Copy link
Member

@jakebailey jakebailey commented Mar 7, 2025

Includes #424, but still has other problems, such as:

  • TestCompilerBaselinesSubmodule/compiler/unreachableDeclarations.ts_preserveconstenums=false/error is nondeterministic.
    • Our SourceFileAffecting list did not consider the options the binder uses.
  • In hereby tests --concurrentTestPrograms, concurrent checking causes problems for any diagnostics that works cross-file, like duplicate identifiers, mismatching type params.
    • TestCompilerBaselinesSubmodule/compiler/jsFileCompilationDuplicateFunctionImplementation.ts/error
    • TestCompilerBaselinesSubmodule/conformance/duplicateNumericIndexers.ts/error
  • Similarly in that mode, some tests appear to be missing exports from other files.
    • TestCompilerBaselinesSubmodule/conformance/nodeModulesSynchronousCallErrors.ts_module=node16/error

@jakebailey jakebailey force-pushed the jabaile/error-baselines branch from 8452ada to 4edaa88 Compare March 7, 2025 18:44
@jakebailey
Copy link
Member Author

Part of the problem is that when we check files, we can issue errors on files other than those being checked. When we are using multiple checkers, we have to make sure to ask all other checkers for their errors too, that fixed about half of the failures. The rest are some other problem.

@jakebailey
Copy link
Member Author

jakebailey commented Mar 7, 2025

  • recursiveExportAssignmentAndFindAliasedType1 - This one seems like it'll be hard to fix; this is a circularity and so by asking all checkers for their diags, we get multiple locations for the same circularity.
    • Same for recursiveExportAssignmentAndFindAliasedType2, recursiveExportAssignmentAndFindAliasedType3, circular1, circular2,
  • unusedTypeParameters7 - this test merges two declarations with type parameters, with multiple checkers, the checkers don't share info about whether or not the type parameters are used, so in concurrent mode we get an unused type declaration error.
  • superInStaticMembers1(target=es2015) - I think this is a case where there's a stronger error from having shadowed reserved names, but when we check another referencing file, we instead issue a different relationship error instead, whose text span is wider than the "original" error, and so we omit the one inside it...
  • typeOnlyMerge2, typeOnlyMerge3 - No clue; it's like we are not detecting that something is a type import. This test is really odd because somehow there's a type error without a location, even in Strada... So something here is cursed both ways.

@jakebailey
Copy link
Member Author

Other than those failures, though, everything does actually seem to be working with concurrent checking.

@jakebailey
Copy link
Member Author

After talking with @ahejlsberg, it seems like the first one is inherent and mostly unsolvable; we probably will have to just have a concurrent variant or just skip the test in concurrent mode.

For unusedTypeParameters7, the only actual solution would be to share usage checks between checkers, but that seems bad, so we might just disable usage checks for declarations that cross file boundaries.

The others, who knows 😄

@jakebailey
Copy link
Member Author

Daniel's 3 PRs reduced the number of differences by some 60k lines. Awesome.

@jakebailey
Copy link
Member Author

With main merged in, down another 20k lines of diffs.

@jakebailey jakebailey marked this pull request as ready for review March 10, 2025 16:06
@jakebailey
Copy link
Member Author

I've updated the PR to skip the tests which fail in concurrent mode for now. This should be ready to go in.

@jakebailey jakebailey added this pull request to the merge queue Mar 10, 2025
Merged via the queue into main with commit a3ecec2 Mar 10, 2025
17 checks passed
@jakebailey jakebailey deleted the jabaile/error-baselines branch March 10, 2025 17:02
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.

2 participants