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

feat(25259): Better error report for equals instead of colon in object literals #38754

Merged
merged 1 commit into from
Jun 30, 2020

Conversation

a-tarasyuk
Copy link
Contributor

Fixes #25259

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented May 26, 2020

This is nice! But I think part of what @JoshuaKGoldberg had in mind as a UX problem was that the left side of the = tries to resolve to a local variable.

So when we issue this error, we should also avoid resolving the left side, and instead treat that as a property declaration on the object instead of as a local binding name.

@a-tarasyuk a-tarasyuk marked this pull request as draft May 27, 2020 07:33
@a-tarasyuk
Copy link
Contributor Author

@DanielRosenwasser Thanks for the feedback. For the following example, TS throws two errors

const a = {
    x: 1,
    y = 1
    ~ ~
}
TS18004: No value exists in scope for the shorthand property 'y'. Either declare one or provide an initializer.
TS1312: '=' can only be used in an object literal property inside a destructuring assignment.

As I understood, the first error should be omitted and the error should be assigned to the property, not to EqualsToken. Am I right?

const a = {
    x: 1,
    y = 1
    ~~~~~
}
TS1312: '=' can only be used in an object literal property inside a destructuring assignment. Did you mean to use a ':'?

@DanielRosenwasser
Copy link
Member

That's correct, though I don't feel that strongly about issuing the error on the = token vs the property itself.

@a-tarasyuk a-tarasyuk marked this pull request as ready for review May 29, 2020 11:03
@a-tarasyuk a-tarasyuk force-pushed the feat/25259 branch 2 times, most recently from 971fa5f to 432ad50 Compare June 2, 2020 06:59
@DanielRosenwasser
Copy link
Member

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 2, 2020

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 2, 2020

Hey @DanielRosenwasser, 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/75512/artifacts?artifactName=tgz&fileId=A4683FE141404FDB3E0BAA1F2DFF6F1A3D5D130C6BBB52E5FF7913A0EA015B4902&fileName=/typescript-4.0.0-insiders.20200602.tgz"
    }
}

and then running npm install.


There is also a playground for this build.

@DanielRosenwasser
Copy link
Member

I'm waiting on a playground build because I want to understand the behavior (e.g. does hovering over the symbol cause issues etc.)

@orta
Copy link
Contributor

orta commented Jun 2, 2020

an hour seems suspicious, let me have a look - yep, a weird issue with setting up the azure CLI will try re-trigger

@DanielRosenwasser
Copy link
Member

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 3, 2020

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 3, 2020

Hey @DanielRosenwasser, 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/75598/artifacts?artifactName=tgz&fileId=87E0D49432E86A9705A3F5C6CB7B34782681EB083097E4CF9293773AA502CD2902&fileName=/typescript-4.0.0-insiders.20200603.tgz"
    }
}

and then running npm install.


There is also a playground for this build.

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Jun 3, 2020

So the following

let x = { foo = 100 }
x.foo

has some strange behavior where find-all-references isn't consistent on any location of foo. Additionally, quick info on x.foo is (property) foo: number, whereas foo = 100 is (property) foo: any.

I'm not totally against this, but how easy would it be to special case this so that the foo = 100 case is internally treated like the foo: 100 case too?

If it is too much of a pain, I would be okay with merging the change. @sheetalkamat, any concerns there?

Also, probably worth adding a test like

let foo = "hello"
let x = { foo = 100 }
x.foo

@a-tarasyuk
Copy link
Contributor Author

a-tarasyuk commented Jun 3, 2020

@DanielRosenwasser

has some strange behavior where find-all-references isn't consistent on any location of foo. Additionally, quick info on x.foo is (property) foo: number, whereas foo = 100 is (property) foo: any

I think this is due to checking the value of the property. { x = 1/**/ infers the type from value } I'm not sure, maybe this should be completely ignored in checkObjectLiteral in favor of checking in checkGrammarObjectLiteralExpression which throws the error, or the name should be checked somehow.

@DanielRosenwasser DanielRosenwasser merged commit 1bbd5ef into microsoft:master Jun 30, 2020
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.

Better error report for equals instead of colon in object literals
5 participants