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

More did-you-mean errors on classes in plain JS #49827

Merged
merged 6 commits into from
Sep 22, 2023

Conversation

sandersn
Copy link
Member

@sandersn sandersn commented Jul 7, 2022

Classes

  • that are declared with class declarations or expressions, but not
    constructor functions
  • and which have nothing in their heritage clauses

Now provide spelling suggestions on misspelled property accesses.

Because of JS support for assignment-as-declaration, it's still easy to mistakenly declare a property with a typo and not get any suggestions.

I'm not sure this is a great idea; I'm going to run user tests and see how they look.

Fixes #45479

Classes
- that are declared with class declarations or expressions, but not
  constructor functions
- and which have nothing in their heritage clauses

Now provide spelling suggestions on misspelled property accesses.

Because of JS support for assignment-as-declaration, it's still easy
to mistakenly declare a property with a typo and not get any
suggestions.
@sandersn
Copy link
Member Author

sandersn commented Jul 7, 2022

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 7, 2022

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 7, 2022

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

@sandersn sandersn self-assigned this Jul 7, 2022
@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Jul 7, 2022
@typescript-bot
Copy link
Collaborator

@sandersn
Great news! no new errors were found between main..refs/pull/49827/merge

@sandersn
Copy link
Member Author

TODO: This should probably skip decorated classes too

@sandersn
Copy link
Member Author

@typescript-bot test top100
@typescript-bot user test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 26, 2023

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 26, 2023

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

Update: The results are in!

@sandersn
Copy link
Member Author

I think this is ready to review, but unless I can think of a better way to test it, I don't want to put it in 5.2 beta; right now my testing plan is to have real people to use and complain if the errors are too much.

Any other ideas? I guess I could artificially turn this on for more file types, but I don't know of any good source of code with typos in it. Maybe I could download random npm packages.

@typescript-bot
Copy link
Collaborator

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

There were infrastructure failures potentially unrelated to your change:

  • 1 instance of "Unknown failure"
  • 1 instance of "Package install failed"

Otherwise...

Something interesting changed - please have a look.

Details

rxjs-src

/mnt/ts_downloads/rxjs-src/build.sh

  • [NEW] error TS2428: All declarations of 'WeakMap' must have identical type parameters.
    • /home/vsts/work/1/s/typescript-49827/lib/lib.es2015.collection.d.ts(63,11)
    • /home/vsts/work/1/s/typescript-49827/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-49827/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-49827/lib/lib.es2015.collection.d.ts(63,11)
    • /home/vsts/work/1/s/typescript-49827/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-49827/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-49827/lib/lib.es2015.collection.d.ts(63,11)
    • /home/vsts/work/1/s/typescript-49827/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-49827/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-49827/lib/lib.es2015.collection.d.ts(63,11)
    • /home/vsts/work/1/s/typescript-49827/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-49827/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-49827/lib/lib.es2015.collection.d.ts(63,11)
    • /home/vsts/work/1/s/typescript-49827/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-49827/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
  • [MISSING] error TS2428: All declarations of 'WeakMap' must have identical type parameters.
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.collection.d.ts(63,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.collection.d.ts(63,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.collection.d.ts(63,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.collection.d.ts(63,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.symbol.wellknown.d.ts(140,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.collection.d.ts(63,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.iterable.d.ts(162,11)
    • /home/vsts/work/1/s/typescript-main/lib/lib.es2015.symbol.wellknown.d.ts(140,11)

Copy link
Member

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

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

This seems fairly conservative, and I can’t think of a great way to test it on real code, since typos tend to get fixed eventually.

@typescript-bot
Copy link
Collaborator

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

Everything looks good!

@sandersn
Copy link
Member Author

Yeah, I agree. I'll merge it when 5.3 development starts.

@sandersn
Copy link
Member Author

Later: Or, it turns out, just before it ends.

@sandersn sandersn merged commit 982f8be into main Sep 22, 2023
19 checks passed
@sandersn sandersn deleted the more-dym-plainjs-errors-on-classes branch September 22, 2023 23:12
snovader pushed a commit to mestro-se/TypeScript that referenced this pull request Sep 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

No suggestion diagnostic for unchecked JS files in method from class
3 participants