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

Error on types named "undefined" #57575

Merged
merged 16 commits into from
Mar 11, 2024
Merged

Conversation

jakebailey
Copy link
Member

@jakebailey jakebailey commented Feb 28, 2024

Fixes #57573

This makes us error on types named undefined in the same way we do keyword types. Though undefined is not a keyword, it is a keyword in type space.

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Feb 28, 2024
@jakebailey
Copy link
Member Author

@typescript-bot test top400
@typescript-bot user test this
@typescript-bot run dt

@typescript-bot
Copy link
Collaborator

typescript-bot commented Feb 28, 2024

Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at 7e0ddf4. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Feb 28, 2024

Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at 7e0ddf4. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Feb 28, 2024

Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at 7e0ddf4. You can monitor the build here.

Update: The results are in!

@dragomirtitian
Copy link
Contributor

Sorta curious how many people are affected.

(see #57573)

Hopefully nobody 😅

@typescript-bot
Copy link
Collaborator

@jakebailey Here are the results of running the user test suite comparing main and refs/pull/57575/merge:

There were infrastructure failures potentially unrelated to your change:

  • 1 instance of "Package install failed"

Otherwise...

Something interesting changed - please have a look.

Details

puppeteer

packages/browsers/test/src/tsconfig.json

@jakebailey
Copy link
Member Author

FWIW the way I wanted to implement this was "if you see an undefined Identifier in type space (besides a typeof), error", but couldn't figure out the best way to do that, so I just threw errors onto everything that introduced a name to type space instead...

@typescript-bot
Copy link
Collaborator

Hey @jakebailey, the results of running the DT tests are ready.
Everything looks the same!
You can check the log here.

@typescript-bot
Copy link
Collaborator

@jakebailey Here are the results of running the top-repos suite comparing main and refs/pull/57575/merge:

Everything looks good!

@jakebailey
Copy link
Member Author

Good, nobody!

@jakebailey
Copy link
Member Author

Well, given #57573 is now effectively accepted, I guess I'll clean this up?

@jakebailey
Copy link
Member Author

Apparently I'm dumb and didn't realize that we already issue this error:

==== undefinedTypeAssignment4.ts (6 errors) ====
    class undefined {
         ~~~~~~~~~~
!!! error TS2397: Declaration name conflicts with built-in global identifier 'undefined'.
          ~~~~~~~~~
!!! error TS2397: Declaration name conflicts with built-in global identifier 'undefined'.
    	foo: string;
    }
    interface undefined {

I just added a duplicate one. The actual problem are the other ones we're missing.

@jakebailey
Copy link
Member Author

Yeah, I did not test what I thought I was.

@jakebailey
Copy link
Member Author

Okay, I sorta tested the right thing. This check is duplicative, but I didn't ignore this in globals, so I did actually check the right thing, kinda.

@jakebailey

This comment was marked as outdated.

@typescript-bot

This comment was marked as outdated.

@typescript-bot

This comment was marked as outdated.

@typescript-bot

This comment was marked as outdated.

@jakebailey jakebailey changed the title Error on new type-ish names named undefined Error on types named "undefined" like type keywords Feb 29, 2024
@jakebailey
Copy link
Member Author

@typescript-bot test top400
@typescript-bot user test this
@typescript-bot run dt

@typescript-bot
Copy link
Collaborator

typescript-bot commented Feb 29, 2024

Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at b376d0b. You can monitor the build here.

Update: The results are in!

@jakebailey jakebailey marked this pull request as ready for review March 1, 2024 19:26
@jakebailey
Copy link
Member Author

Okay, this should be ready. Removed the namespace stuff as discussed.

@jakebailey
Copy link
Member Author

@typescript-bot test top400
@typescript-bot user test this
@typescript-bot run dt

@typescript-bot perf test this faster
@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 1, 2024

Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at 7b80d7a. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 1, 2024

Heya @jakebailey, I've started to run the faster perf test suite on this PR at 7b80d7a. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 1, 2024

Heya @jakebailey, I've started to run the tarball bundle task on this PR at 7b80d7a. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 1, 2024

Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at 7b80d7a. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 1, 2024

Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at 7b80d7a. You can monitor the build here.

@@ -2720,20 +2717,19 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
}
}

function addToSymbolTable(target: SymbolTable, source: SymbolTable, message: DiagnosticMessage) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opted to eliminate builtinGlobals while here; its only use is to complain about undefined, but now we're going to complain even less and the new code had to exclude type declarations anyway.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 1, 2024

Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/160140/artifacts?artifactName=tgz&fileId=A08C03FF21AB9BAC355C636691B79EDA68D4896F6685371FEA3779F5838F653802&fileName=/typescript-5.5.0-insiders.20240301.tgz"
    }
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@5.5.0-pr-57575-41".;

@typescript-bot
Copy link
Collaborator

Hey @jakebailey, the results of running the DT tests are ready.
Everything looks the same!
You can check the log here.

@typescript-bot
Copy link
Collaborator

@jakebailey
The results of the perf run you requested are in!

Here they are:

tsc

Comparison Report - baseline..pr
Metric baseline pr Delta Best Worst p-value
Angular - node (v18.15.0, x64)
Memory used 295,571k (± 0.01%) 295,564k (± 0.01%) ~ 295,531k 295,578k p=0.471 n=6
Parse Time 2.66s (± 0.15%) 2.66s (± 0.37%) ~ 2.65s 2.67s p=0.930 n=6
Bind Time 0.82s (± 0.50%) 0.82s (± 0.63%) ~ 0.82s 0.83s p=0.595 n=6
Check Time 8.22s (± 0.45%) 8.21s (± 0.24%) ~ 8.19s 8.24s p=0.334 n=6
Emit Time 7.11s (± 0.48%) 7.11s (± 0.29%) ~ 7.09s 7.15s p=0.745 n=6
Total Time 18.83s (± 0.26%) 18.81s (± 0.19%) ~ 18.78s 18.88s p=0.806 n=6
Compiler-Unions - node (v18.15.0, x64)
Memory used 193,222k (± 1.00%) 192,093k (± 0.73%) ~ 191,419k 194,945k p=0.471 n=6
Parse Time 1.36s (± 1.02%) 1.36s (± 0.72%) ~ 1.35s 1.38s p=0.203 n=6
Bind Time 0.72s (± 0.00%) 0.72s (± 0.00%) ~ 0.72s 0.72s p=1.000 n=6
Check Time 9.36s (± 0.73%) 9.38s (± 0.67%) ~ 9.30s 9.49s p=0.688 n=6
Emit Time 2.60s (± 0.62%) 2.61s (± 0.45%) ~ 2.59s 2.62s p=0.359 n=6
Total Time 14.05s (± 0.62%) 14.07s (± 0.51%) ~ 13.98s 14.19s p=0.630 n=6
Monaco - node (v18.15.0, x64)
Memory used 347,340k (± 0.00%) 347,335k (± 0.01%) ~ 347,296k 347,371k p=0.810 n=6
Parse Time 2.48s (± 0.66%) 2.48s (± 0.40%) ~ 2.47s 2.49s p=0.557 n=6
Bind Time 0.93s (± 0.59%) 0.92s (± 0.59%) ~ 0.92s 0.93s p=1.000 n=6
Check Time 6.94s (± 0.44%) 6.93s (± 0.27%) ~ 6.90s 6.95s p=0.467 n=6
Emit Time 4.07s (± 0.31%) 4.08s (± 0.31%) ~ 4.06s 4.09s p=0.211 n=6
Total Time 14.41s (± 0.32%) 14.42s (± 0.16%) ~ 14.39s 14.45s p=0.872 n=6
TFS - node (v18.15.0, x64)
Memory used 302,757k (± 0.00%) 302,766k (± 0.01%) ~ 302,739k 302,784k p=0.149 n=6
Parse Time 2.01s (± 0.63%) 2.01s (± 0.52%) ~ 2.00s 2.03s p=0.615 n=6
Bind Time 1.00s (± 0.55%) 1.01s (± 1.73%) +0.02s (+ 2.01%) 1.00s 1.05s p=0.007 n=6
Check Time 6.35s (± 0.22%) 6.35s (± 0.44%) ~ 6.32s 6.39s p=1.000 n=6
Emit Time 3.60s (± 0.60%) 3.61s (± 0.54%) ~ 3.57s 3.62s p=0.418 n=6
Total Time 12.95s (± 0.27%) 12.99s (± 0.34%) ~ 12.93s 13.03s p=0.195 n=6
material-ui - node (v18.15.0, x64)
Memory used 511,206k (± 0.01%) 511,197k (± 0.00%) ~ 511,172k 511,228k p=0.575 n=6
Parse Time 2.65s (± 0.57%) 2.65s (± 0.52%) ~ 2.63s 2.67s p=1.000 n=6
Bind Time 0.99s (± 0.76%) 0.98s (± 0.83%) ~ 0.97s 0.99s p=0.120 n=6
Check Time 17.28s (± 0.32%) 17.27s (± 0.45%) ~ 17.19s 17.39s p=0.808 n=6
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) ~ 0.00s 0.00s p=1.000 n=6
Total Time 20.92s (± 0.31%) 20.91s (± 0.40%) ~ 20.79s 21.02s p=0.872 n=6
mui-docs - node (v18.15.0, x64)
Memory used 2,295,511k (± 0.00%) 2,295,510k (± 0.00%) ~ 2,295,480k 2,295,548k p=1.000 n=6
Parse Time 11.97s (± 0.97%) 11.97s (± 0.64%) ~ 11.88s 12.09s p=0.810 n=6
Bind Time 2.65s (± 0.15%) 2.65s (± 0.21%) ~ 2.64s 2.65s p=0.282 n=6
Check Time 101.58s (± 0.56%) 101.33s (± 0.69%) ~ 100.38s 102.40s p=0.471 n=6
Emit Time 0.32s (± 0.00%) 0.32s (± 1.27%) ~ 0.32s 0.33s p=0.405 n=6
Total Time 116.51s (± 0.46%) 116.27s (± 0.56%) ~ 115.43s 117.29s p=0.575 n=6
self-build-src - node (v18.15.0, x64)
Memory used 2,395,105k (± 0.01%) 2,394,710k (± 0.02%) ~ 2,394,186k 2,395,572k p=0.173 n=6
Parse Time 5.05s (± 0.81%) 5.04s (± 0.32%) ~ 5.01s 5.05s p=0.936 n=6
Bind Time 1.88s (± 0.48%) 1.89s (± 0.97%) ~ 1.87s 1.92s p=0.284 n=6
Check Time 33.36s (± 0.27%) 33.55s (± 0.32%) +0.19s (+ 0.57%) 33.44s 33.70s p=0.013 n=6
Emit Time 2.69s (± 1.12%) 2.69s (± 1.30%) ~ 2.63s 2.73s p=0.575 n=6
Total Time 43.01s (± 0.15%) 43.18s (± 0.22%) +0.17s (+ 0.40%) 43.03s 43.29s p=0.020 n=6
self-compiler - node (v18.15.0, x64)
Memory used 414,367k (± 0.01%) 414,366k (± 0.01%) ~ 414,332k 414,448k p=0.810 n=6
Parse Time 2.80s (± 0.89%) 2.78s (± 1.75%) ~ 2.71s 2.85s p=0.746 n=6
Bind Time 1.06s (± 0.38%) 1.09s (± 5.32%) +0.03s (+ 2.83%) 1.06s 1.21s p=0.033 n=6
Check Time 15.11s (± 0.44%) 15.12s (± 0.46%) ~ 15.01s 15.21s p=0.810 n=6
Emit Time 1.11s (± 0.68%) 1.11s (± 1.39%) ~ 1.09s 1.12s p=0.796 n=6
Total Time 20.08s (± 0.38%) 20.10s (± 0.54%) ~ 19.96s 20.25s p=0.689 n=6
vscode - node (v18.15.0, x64)
Memory used 2,854,203k (± 0.00%) 2,854,196k (± 0.00%) ~ 2,854,165k 2,854,239k p=0.936 n=6
Parse Time 10.78s (± 0.32%) 10.74s (± 0.26%) ~ 10.70s 10.78s p=0.107 n=6
Bind Time 3.42s (± 0.22%) 3.43s (± 0.22%) ~ 3.42s 3.44s p=0.062 n=6
Check Time 60.79s (± 0.49%) 60.82s (± 0.47%) ~ 60.53s 61.29s p=0.810 n=6
Emit Time 16.24s (± 0.68%) 16.20s (± 0.66%) ~ 16.04s 16.32s p=0.471 n=6
Total Time 91.23s (± 0.40%) 91.20s (± 0.37%) ~ 90.82s 91.74s p=0.936 n=6
webpack - node (v18.15.0, x64)
Memory used 396,845k (± 0.03%) 396,841k (± 0.01%) ~ 396,782k 396,891k p=0.810 n=6
Parse Time 3.13s (± 0.89%) 3.14s (± 0.62%) ~ 3.11s 3.16s p=0.685 n=6
Bind Time 1.37s (± 1.07%) 1.38s (± 0.76%) ~ 1.37s 1.40s p=0.073 n=6
Check Time 13.98s (± 0.15%) 13.97s (± 0.29%) ~ 13.92s 14.04s p=0.332 n=6
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) ~ 0.00s 0.00s p=1.000 n=6
Total Time 18.48s (± 0.25%) 18.49s (± 0.21%) ~ 18.42s 18.54s p=0.872 n=6
xstate - node (v18.15.0, x64)
Memory used 513,147k (± 0.00%) 513,186k (± 0.01%) ~ 513,095k 513,295k p=0.298 n=6
Parse Time 3.27s (± 0.30%) 3.27s (± 0.25%) ~ 3.26s 3.28s p=0.862 n=6
Bind Time 1.54s (± 0.49%) 1.54s (± 0.54%) ~ 1.52s 1.54s p=0.652 n=6
Check Time 2.86s (± 0.51%) 2.87s (± 0.48%) ~ 2.85s 2.89s p=0.255 n=6
Emit Time 0.08s (± 4.99%) 0.08s (± 0.00%) ~ 0.08s 0.08s p=0.405 n=6
Total Time 7.75s (± 0.15%) 7.76s (± 0.15%) ~ 7.75s 7.78s p=0.217 n=6
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • Angular - node (v18.15.0, x64)
  • Compiler-Unions - node (v18.15.0, x64)
  • Monaco - node (v18.15.0, x64)
  • TFS - node (v18.15.0, x64)
  • material-ui - node (v18.15.0, x64)
  • mui-docs - node (v18.15.0, x64)
  • self-build-src - node (v18.15.0, x64)
  • self-compiler - node (v18.15.0, x64)
  • vscode - node (v18.15.0, x64)
  • webpack - node (v18.15.0, x64)
  • xstate - node (v18.15.0, x64)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

Developer Information:

Download Benchmarks

@typescript-bot
Copy link
Collaborator

@jakebailey Here are the results of running the top-repos suite comparing main and refs/pull/57575/merge:

Everything looks good!

@jakebailey
Copy link
Member Author

Abusing this PR to try out a cherry-pick with the new bot.

@typescript-bot cherry-pick this to release-5.4

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 2, 2024

Starting jobs; this comment will be updated as builds start and complete.

Command Status Results
cherry-pick this to release-5.4 ✅ Started ✅ Results

@typescript-bot
Copy link
Collaborator

Hey, @jakebailey! I've created #57610 for you.

@sandersn sandersn added this to Not started in PR Backlog Mar 8, 2024
@jakebailey jakebailey moved this from Not started to Waiting on reviewers in PR Backlog Mar 8, 2024
PR Backlog automation moved this from Waiting on reviewers to Needs merge Mar 9, 2024
@jakebailey jakebailey added the Breaking Change Would introduce errors in existing code label Mar 11, 2024
@jakebailey jakebailey merged commit e24d886 into microsoft:main Mar 11, 2024
19 checks passed
PR Backlog automation moved this from Needs merge to Done Mar 11, 2024
@jakebailey jakebailey deleted the ban-undefined branch March 11, 2024 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team Breaking Change Would introduce errors in existing code For Backlog Bug PRs that fix a backlog bug
Projects
No open projects
PR Backlog
  
Done
Development

Successfully merging this pull request may close these issues.

A type can be named undefined
4 participants