Improve error messages for property declarations #5559

MartyIX · 2015-11-07T01:04:41Z

Proposed fix for #4045.

I'm not entirely sure if I'm on the right track. It's more complicated than I expected.

mhegazy · 2015-11-07T01:14:54Z

i do not think this is the right way to accomplish this change. you probably want to allow parsing const as a modifier for property declarations, and then give an error for it later on (i.e. in checker::checkGrammarModifiers).

@CyrusNajmabadi or @vladima can help you more here.

MartyIX · 2015-11-07T01:22:41Z

@mhegazy I tried the approach that Daniel mentioned (maybe I did not understand him though). He mentions there a more complex example than the original issue is about. I'll certainly appreciate any feedback.

CyrusNajmabadi · 2015-11-07T01:27:06Z

I agree with @mhegazy .

We know we're in parseClassElement. In that case, just pass a flag to parseClassModifiers saying that 'const should be treated as a modifier."

Easy peasy. Then later during grammar checks, disallow 'const' on anything other than an enum.

MartyIX · 2015-11-08T11:41:00Z

It's definitely more easy peasy for you. 😄 However, I tried to move it a bit further.

I don't know how to fix the fourslash test. I don't understand what verify.occurrencesAtPositionCount(1); command actually checks.

MartyIX · 2015-11-09T21:32:44Z

@mhegazy Can I ask you for feedback?

mhegazy · 2015-11-09T22:59:20Z

src/compiler/parser.ts

            let flags = 0;
            let modifiers: ModifiersArray;
            while (true) {
                const modifierStart = scanner.getStartPos();
                const modifierKind = token;

-                if (!parseAnyContextualModifier()) {
-                    break;
+                if (whitelist && whitelist.indexOf(token) >= 0) {


use ts.indexOf instead

Yes, I have already found it (#5559 (comment))

CyrusNajmabadi · 2015-11-10T19:49:38Z

src/compiler/parser.ts

@@ -1124,6 +1124,14 @@ namespace ts {
            return token === t && tryParse(nextTokenCanFollowModifier);
        }

+        function nextTokenIsOnSameLineAndCanFollowModifier() {


So we have nextTokenIsOnSameLineAndCanFollowModifier and nextTokenCanFollowModifier...

These are supremely easy to confuse. When would i want one, versus the other?

It was nextTokenCanFollowModifierWithoutGrammarRules before. It seemed more clear to me. Basically, I just need nextTokenCanFollowModifier without the leading checks. I can think of several solutions here:

Inline nextTokenIsOnSameLineAndCanFollowModifier to parseModifiers (It leads to code duplication).

Rename nextTokenIsOnSameLineAndCanFollowModifier and use it in nextTokenCanFollowModifier as now.

Somehow pass a flag to tryParse. It seems like a big change because tryParse is a generic function that is used everywhere.

The option 2 seems reasonable overall.

But why do some people needs checks, and others do not? That's the part I find confusing. How would I know from my code?

Well, to be frank, I don't know why the if statements with SyntaxKind in the following code are not a part of checker.ts file:

function nextTokenCanFollowModifier() { if (token === SyntaxKind.ConstKeyword) { // 'const' is only a modifier if followed by 'enum'. return nextToken() === SyntaxKind.EnumKeyword; } if (token === SyntaxKind.ExportKeyword) { nextToken(); if (token === SyntaxKind.DefaultKeyword) { return lookAhead(nextTokenIsClassOrFunction); } return token !== SyntaxKind.AsteriskToken && token !== SyntaxKind.OpenBraceToken && canFollowModifier(); } if (token === SyntaxKind.DefaultKeyword) { return nextTokenIsClassOrFunction(); } if (token === SyntaxKind.StaticKeyword) { nextToken(); return canFollowModifier(); } nextToken(); if (scanner.hasPrecedingLineBreak()) { return false; } return canFollowModifier(); }

That way, one would always have all modifiers for a property or a class (possibly something else?) and they would be checked in checker.ts if they are correct. Then there would be no need for special handling of const which now seems a bit like a hack. But maybe I'm missing something because I don't know have deep knowledge of the compiler.

I think what Cyrus is sort of getting at is that more than anything else, you should be prescriptive and document why this function exists and where it should be used.

MartyIX · 2015-11-13T20:01:53Z

@mhegazy @DanielRosenwasser Do you have any suggestions how to make progress on this issue?

DanielRosenwasser · 2015-11-18T09:08:54Z

src/compiler/parser.ts

@@ -4940,7 +4951,7 @@ namespace ts {

            const fullStart = getNodePos();
            const decorators = parseDecorators();
-            const modifiers = parseModifiers();
+            const modifiers = parseModifiers(true /* permitConstAsModifier */);


the comment will need to be moved to the front

DanielRosenwasser · 2015-11-18T09:20:12Z

@MartyIX is there a test with something like

class C {
    const
    x = 10;
}

MartyIX · 2015-11-18T22:31:40Z

@DanielRosenwasser I added tests/cases/compiler/ClassDeclarationWithInvalidConstOnPropertyDeclaration2.ts.

DanielRosenwasser · 2015-11-18T22:56:42Z

Looks like there is a fourslash test failure for tests/cases/fourslash/getOccurrencesConst04.ts. Can you check it out?

MartyIX · 2015-11-18T23:00:30Z

Yes, I know. I mentioned it here #5559 (comment) Some documentation for fourslash tests would be helpful.

DanielRosenwasser · 2015-11-18T23:44:22Z

Oh sorry, I missed that. We provide something called "occurrences highlighting" or "highlight spans". When the caret is placed on top of certain identifiers and keywords, we'll highlight relevant matches:

So with verify.occurrencesAtPositionCount(1); we're ensuring that only a single keyword gets highlighted. If that's the failure point, something is not working. If you have the Sublime Plugin or Visual Studio, you can update the language services (see here how to do it with VS).

MartyIX · 2015-11-20T19:01:03Z

@DanielRosenwasser Thanks

MartyIX · 2015-11-24T06:54:04Z

@mhegazy @DanielRosenwasser Can you have a look at the code?

DanielRosenwasser · 2015-11-24T08:49:57Z

tests/cases/fourslash/getOccurrencesConst04.ts

@@ -1,7 +1,7 @@
 /// <reference path='fourslash.ts' />

 ////export const class C {
-////    private static c/*1*/onst foo;
+////    private static const f/*1*/oo;


The fix shouldn't be to change the marker position, it should be to ensure that the occurrence count is 0.

DanielRosenwasser · 2015-11-24T09:00:43Z

For some reason, we aren't observing the new error message on the following:

class C {
    private const foo;

    constructor(const) {
    }
}

We report an error on the const in parameter position, but nothing on the declaration for foo. Any ideas @CyrusNajmabadi @MartyIX?

CyrusNajmabadi · 2015-11-24T09:30:49Z

Have you tried debugging through it?

MartyIX · 2015-11-24T09:32:14Z

I'll have a look in a few hours (as soon as I finish some other work).

MartyIX · 2015-11-24T14:01:28Z

@DanielRosenwasser I added console.logs to parseErrorAtPosition and grammarErrorOnNode and I got the following output for your snippet:

$ node built/local/tsc.js hello.ts 
parseErrorAtPosition { code: 1003,
  category: 1,
  key: 'Identifier_expected_1003',
  message: 'Identifier expected.' }
hello.ts(4,17): error TS1003: Identifier expected.
grammarErrorOnNode { code: 1246,
  category: 1,
  key: 'A_class_member_cannot_have_the_0_keyword_1246',
  message: 'A class member cannot have the \'{0}\' keyword.' }

So parseErrorAtPosition is called and grammarErrorOnNode is called too. However, tsc.ts contains:

// If we didn't have any syntactic errors, then also try getting the global and
// semantic errors.

https://github.com/Microsoft/TypeScript/blob/master/src/compiler/tsc.ts#L568

The conclusion is that semantic errors are not shown when parser errors occur.

MartyIX · 2015-11-27T10:48:52Z

@DanielRosenwasser @mhegazy Is this behavior OK then? Or can I make the PR better?

DanielRosenwasser · 2015-11-27T18:51:39Z

src/compiler/checker.ts

@@ -15324,6 +15324,11 @@ namespace ts {
            let flags = 0;
            for (const modifier of node.modifiers) {
                switch (modifier.kind) {
+                    case SyntaxKind.ConstKeyword:
+                        if (node.parent.kind === SyntaxKind.ClassDeclaration && node.kind !== SyntaxKind.EnumDeclaration) {


Can you switch the condition around to check for node.kind first?

DanielRosenwasser · 2015-11-27T18:52:42Z

Other than the last comment I left, looks good to me!

MartyIX · 2015-11-27T19:12:28Z

@DanielRosenwasser Ah, sorry. I think you have already mentioned that. I've just fixed it

MartyIX · 2015-12-03T20:06:40Z

@mhegazy @DanielRosenwasser Can this be merged then?

DanielRosenwasser · 2015-12-04T00:33:18Z

Sorry about that, I must have missed the alert. Can you merge from master into your branch and resolve the conflicts?

MartyIX · 2015-12-04T08:32:00Z

@DanielRosenwasser I've updated it.

MartyIX · 2015-12-07T08:21:57Z

@mhegazy @DanielRosenwasser Ping :)

(I'm not in a hurry to get this merged, it's just I don't want to deal with potential unnecessary conflicts.)

Improve error messages for property declarations

DanielRosenwasser · 2015-12-07T09:12:33Z

Sorry about that, it's easy to miss in the flood of notifications, but thanks @MartyIX!

msftclas added the cla-already-signed label Nov 7, 2015

MartyIX changed the title ~~Improved error messages for property declarations~~ Improve error messages for property declarations Nov 7, 2015

MartyIX force-pushed the issue-4045 branch 3 times, most recently from 06e3cbe to 9f393fa Compare November 8, 2015 11:39

mhegazy reviewed Nov 9, 2015
View reviewed changes

MartyIX force-pushed the issue-4045 branch from 9f393fa to 2789a29 Compare November 10, 2015 16:31

CyrusNajmabadi reviewed Nov 10, 2015
View reviewed changes

DanielRosenwasser reviewed Nov 18, 2015
View reviewed changes

MartyIX force-pushed the issue-4045 branch from 2789a29 to bf983cc Compare November 18, 2015 22:21

MartyIX force-pushed the issue-4045 branch 2 times, most recently from 41ba303 to b6871c9 Compare November 20, 2015 17:28

DanielRosenwasser reviewed Nov 24, 2015
View reviewed changes

MartyIX force-pushed the issue-4045 branch from b6871c9 to a285bb6 Compare November 24, 2015 14:46

DanielRosenwasser reviewed Nov 27, 2015
View reviewed changes

MartyIX force-pushed the issue-4045 branch from a285bb6 to 9275b1d Compare November 27, 2015 19:10

MartyIX force-pushed the issue-4045 branch from 9275b1d to ca70fb7 Compare December 4, 2015 08:10

Improved error messages for property declarations

f15fe5b

MartyIX force-pushed the issue-4045 branch from ca70fb7 to f15fe5b Compare December 4, 2015 08:21

DanielRosenwasser added a commit that referenced this pull request Dec 7, 2015

Merge pull request #5559 from MartyIX/issue-4045

2ef436f

Improve error messages for property declarations

DanielRosenwasser merged commit 2ef436f into microsoft:master Dec 7, 2015

microsoft locked and limited conversation to collaborators Jun 19, 2018

Improve error messages for property declarations #5559

Improve error messages for property declarations #5559

Conversation

MartyIX commented Nov 7, 2015

mhegazy commented Nov 7, 2015

MartyIX commented Nov 7, 2015

CyrusNajmabadi commented Nov 7, 2015

MartyIX commented Nov 8, 2015

MartyIX commented Nov 9, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartyIX commented Nov 13, 2015

Choose a reason for hiding this comment

DanielRosenwasser commented Nov 18, 2015

MartyIX commented Nov 18, 2015

DanielRosenwasser commented Nov 18, 2015

MartyIX commented Nov 18, 2015

DanielRosenwasser commented Nov 18, 2015

MartyIX commented Nov 20, 2015

MartyIX commented Nov 24, 2015

Choose a reason for hiding this comment

DanielRosenwasser commented Nov 24, 2015

CyrusNajmabadi commented Nov 24, 2015

MartyIX commented Nov 24, 2015

MartyIX commented Nov 24, 2015

MartyIX commented Nov 27, 2015

Choose a reason for hiding this comment

DanielRosenwasser commented Nov 27, 2015

MartyIX commented Nov 27, 2015

MartyIX commented Dec 3, 2015

DanielRosenwasser commented Dec 4, 2015

MartyIX commented Dec 4, 2015

MartyIX commented Dec 7, 2015

DanielRosenwasser commented Dec 7, 2015