Skip to content

Expand constraint suggestion related span and add quick fix #49481

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 4 commits into from
Jun 15, 2022

Conversation

weswigham
Copy link
Member

This expands the originally very narrow elaboration to be reported any time an unconstrained type parameter is related to something, and adds a quickfix to apply the change.

@RyanCavanaugh
Copy link
Member

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 10, 2022

Heya @RyanCavanaugh, I've started to run the tarball bundle task on this PR at 00ef5ae. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 10, 2022

Hey @RyanCavanaugh, 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/127860/artifacts?artifactName=tgz&fileId=598827D61D0342AD0F9999019F7660B392F62AE485A78FB74B15AED9C2CF8C1F02&fileName=/typescript-4.8.0-insiders.20220610.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/[email protected]".;

@RyanCavanaugh
Copy link
Member

Most of the breaks I saw looked like this, which currently isn't quickfixable. Can we expand it?

interface Fn<T extends string> {
}

function m<T>(x: Fn<T>) {
}

@weswigham
Copy link
Member Author

Can we expand it?

Sure, should be easy - just another error code in the quickfix hopefully, since it uses a custom head message on the assignability error.

@weswigham
Copy link
Member Author

@RyanCavanaugh done. There are a lot of custom head messages for relation errors, and thus, many error codes we could conceivably add to this quickfix. I don't claim the current list to be exhaustive, just the ones I read in the core reportRelationError function. 😅

@RyanCavanaugh
Copy link
Member

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 10, 2022

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 10, 2022

Hey @RyanCavanaugh, 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/127897/artifacts?artifactName=tgz&fileId=32B3A22AB17DD45B6358503B57C35FED620753775DE1BF65C16A33E45EAF8FAD02&fileName=/typescript-4.8.0-insiders.20220610.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/[email protected]".;

@andrewbranch
Copy link
Member

(This isn’t the first time it would have been nice to be able to register a codefix against a related error instead of the head message.)

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.

We should keep an eye out for how often the related diagnostic / quick fix is the correct fix in real world code since it seems to be showing up a lot more often.

@weswigham
Copy link
Member Author

We should keep an eye out for how often the related diagnostic / quick fix is the correct fix in real world code since it seems to be showing up a lot more often.

It's any time you assign an unconstrained type parameter to something, so potentially pretty often. Conversely, it's probably got a pretty high success rate, I think. The other way you'd have to take the fix is changing the target type's position to unknown, which seems.... probably not what you wanted if you're using type parameters. I think the misses are moreso places where it suggests a constraint, and then the suggested constraint is insufficient because the source is related to multiple target types (but we don't know that yet because we bail on the first error we find).

@weswigham weswigham merged commit ba38fe1 into microsoft:main Jun 15, 2022
@weswigham
Copy link
Member Author

Notably, that failure mode does imply another, narrower, potentially useful quickfix - if a constrained type parameter fails being related to some target type, suggesting the target type be intersected into the constraint of the source type parameter is potentially a good fix. Unfortunately, a large corpus of code with errors isn't exactly something we have to know how useful such a fix would be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants