Added implementation for issue #25002 #25415

bashdx · 2018-07-03T19:45:19Z

Fixes #
Changed error message for get and set accessors having different types. Also adding a related span with further explanations.

mhegazy · 2018-07-03T20:47:32Z

lib/enu/diagnosticMessages.generated.json.lcg

@@ -6323,7 +6323,7 @@
            </Item>
            <Item ItemId=";get_and_set_accessor_must_have_the_same_type_2380" ItemType="0" PsrId="306" Leaf="true">
              <Str Cat="Text">
-                <Val><![CDATA['get' and 'set' accessor must have the same type.]]></Val>
+                <Val><![CDATA['get' and 'set' accessor must have the same type '{0}', but this 'get' accessor has the type '{1}'.]]></Val>


this file is auto generated as part of the build. do not change it manually.

mhegazy · 2018-07-03T20:48:15Z

lib/tsc.js

@@ -2942,7 +2942,7 @@ var ts;
        Constructors_for_derived_classes_must_contain_a_super_call: diag(2377, ts.DiagnosticCategory.Error, "Constructors_for_derived_classes_must_contain_a_super_call_2377", "Constructors for derived classes must contain a 'super' call."),
        A_get_accessor_must_return_a_value: diag(2378, ts.DiagnosticCategory.Error, "A_get_accessor_must_return_a_value_2378", "A 'get' accessor must return a value."),
        Getter_and_setter_accessors_do_not_agree_in_visibility: diag(2379, ts.DiagnosticCategory.Error, "Getter_and_setter_accessors_do_not_agree_in_visibility_2379", "Getter and setter accessors do not agree in visibility."),
-        get_and_set_accessor_must_have_the_same_type: diag(2380, ts.DiagnosticCategory.Error, "get_and_set_accessor_must_have_the_same_type_2380", "'get' and 'set' accessor must have the same type."),


same here. just revert changes to all files under lib*

mhegazy · 2018-07-03T21:00:20Z

src/compiler/checker.ts

+                    const typeNameSecondType = typeToString(secondType);
+
+                    const diagnostic: Diagnostic = error(first, message, typeNameSecondType, typeNameFirstType);
+                    diagnostic.relatedInformation = [createDiagnosticForNode(first, Diagnostics.The_respective_set_accessor_has_the_type_0, typeNameSecondType)];


use addRelatedInfo instead.

mhegazy · 2018-07-03T21:01:04Z

src/compiler/diagnosticMessages.json

@@ -2401,6 +2401,10 @@
        "category": "Error",
        "code": 2728
    },
+    "The respective 'set' accessor has the type '{0}'.": {


@DanielRosenwasser any comments on the message text?

mhegazy · 2018-07-03T21:01:38Z

src/compiler/diagnosticMessages.json

@@ -2401,6 +2401,10 @@
        "category": "Error",
        "code": 2728
    },
+    "The respective 'set' accessor has the type '{0}'.": {
+        "category": "Error",


this should be "Message" and not "Error"

mhegazy · 2018-07-03T21:02:17Z

You need to run the tests jake runtests-parallel and accept the baseline changes jake baseline-accept

bashdx · 2018-07-05T14:30:20Z

Just FYI, please check the result. When I changed the code so that the result only displayed the getter the unit tests resulted in an error (expected 3 errors, but got 2).
Now the result looks like this and all tests are passing:

test_accessors.ts:2:9 - error TS2380: 'get' and 'set' accessor must have the same type 'number', but this 'get' accessor has the type 'string'.

2     set foo(value: string) { },
          ~~~

  test_accessors.ts:2:9
    2     set foo(value: string) { },
              ~~~
    The respective 'set' accessor has the type 'number'.

test_accessors.ts:3:9 - error TS2380: 'get' and 'set' accessor must have the same type 'string', but this 'get' accessor has the type 'number'.

3     get foo(): number { return 100; }
          ~~~

  test_accessors.ts:3:9
    3     get foo(): number { return 100; }
              ~~~
    The respective 'set' accessor has the type 'string'.

The statement about the 'set' accessor in the first line is wrong. How to proceed?

mhegazy · 2018-07-05T19:31:39Z

tests/baselines/reference/abstractPropertyNegative.errors.txt

@@ -31,10 +31,12 @@ tests/cases/compiler/abstractPropertyNegative.ts(41,18): error TS2676: Accessors
        abstract m(): string;
        abstract get mismatch(): string;
                     ~~~~~~~~
-!!! error TS2380: 'get' and 'set' accessor must have the same type.
+!!! error TS2380: 'get' and 'set' accessor must have the same type 'number', but this 'get' accessor has the type 'string'.


I think the elaboration `but this 'get' accessor has the type 'string' is not needed. adding a related info is sufficient.

mhegazy · 2018-07-05T19:32:10Z

tests/baselines/reference/abstractPropertyNegative.errors.txt

        abstract set mismatch(val: number); // error, not same type
                     ~~~~~~~~
-!!! error TS2380: 'get' and 'set' accessor must have the same type.
+!!! error TS2380: 'get' and 'set' accessor must have the same type 'string', but this 'get' accessor has the type 'number'.
+!!! related TS2730 tests/cases/compiler/abstractPropertyNegative.ts:11:18: The respective 'set' accessor has the type 'string'.


this elaboration should be different, it should be about the get and not the set since this error is already about the set accessor.

This is what I mentioned in my most recent comment above. As of now (with passing tests) the messages are shown twice and the second message is actually garbage. If I test only for the getter the output looks as desired (that's what I presume), but the tests fail.
So what do we do?

not sure i understand what you mean by "but the tests fail."

The tests from jake runtests-parallel fail.
If I implement the function this way:

function checkAccessorDeclarationTypesIdentical(first: AccessorDeclaration, second: AccessorDeclaration, getAnnotatedType: (a: AccessorDeclaration) => Type | undefined, message: DiagnosticMessage) { const firstType = getAnnotatedType(first); const secondType = getAnnotatedType(second); if (firstType && secondType && !isTypeIdenticalTo(firstType, secondType)) { const typeNameSecondType = typeToString(secondType); if(isGetAccessor(first)){ const diagnostic: Diagnostic = error(first, message, typeNameSecondType); addRelatedInfo(diagnostic, createDiagnosticForNode(first, Diagnostics.The_respective_set_accessor_has_the_type_0, typeNameSecondType)); } } }

The output looks like this:

test_accessors.ts:3:9 - error TS2380: 'get' and 'set' accessor must have the same type 'string'. 3 get foo(): number { return 100; } ~~~ test_accessors.ts:3:9 3 get foo(): number { return 100; } ~~~ The respective 'set' accessor has the type 'string'.

This is how I would expect the output to look like. But now the unit tests fail:

jake runtests-parallel node lib/tsc.js -b src node built/local/tsc.js -b built/local/typescriptServices.tsconfig.json cp -r built/local/typescriptServices.js built/local/typescript.js cp -r built/local/typescriptServices.js.map built/local/typescript.js.map node lib/tsc.js -b src/testRunner node built/local/run.js Discovered 134 unittest suites. Discovering runner-based tests... Discovered 12550 test files in 672ms. Starting to run tests using 4 threads... Batching initial test lists... Batched into 4 groups with approximate total file sizes of 2457413 bytes in each group. (99.6% of total tests batched) [▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․] 3 failing [Worker 0] Unexpected error(s) found. Error list is: [Worker 0] from: 1:8, to: 1:10, message: 'get' and 'set' accessor must have the same type 'number'. [Worker 0] from: 5:8, to: 5:9, message: Type '"30"' is not assignable to type 'number'. Actual number of errors (2) does not match expected number (3) [▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬] ✖ 61200/61208 passing (5m)

What to make of this?

The tests are baseline tests, so they will fail every time the output changes. and that is fine.
change the message, then run the tests (jake runtests-parallel), then accept the new baselines (jake baseline-accept).

mhegazy · 2018-07-06T21:39:51Z

@DanielRosenwasser can you please review the error message changes..

DanielRosenwasser · 2018-07-06T21:31:33Z

src/compiler/checker.ts

+                const typeNameSecondType = typeToString(secondType);
+
+                if(isGetAccessor(first)){
+


Remove newline.

DanielRosenwasser · 2018-07-06T21:42:39Z

src/compiler/diagnosticMessages.json

@@ -1304,7 +1304,7 @@
        "category": "Error",
        "code": 2379
    },
-    "'get' and 'set' accessor must have the same type.": {
+    "'get' and 'set' accessor must have the same type '{0}'.": {


I'm thinking

Accessors must have identical types, but this 'get' accessor has type '{0}' and its respective 'set' accessor has type '{1}'.

with

The respective 'set' accessor is declared here.

However, to be honest, I'm not sure how I feel about this change; I feel like the simpler error message was actually better.

why not just the the 'set' accessor is declared here:

bashdx · 2018-07-08T13:49:31Z

I removed the newlines, what's next?

RyanCavanaugh · 2018-09-05T16:23:09Z

@DanielRosenwasser do we need this?

RyanCavanaugh · 2019-04-25T17:50:08Z

@bashdx if you can get the conflicts resolved and the CI green, I think we can merge this

DanielRosenwasser · 2019-04-25T18:02:43Z

tests/baselines/reference/getAndSetNotIdenticalType2.errors.txt

@@ -15,14 +14,13 @@ tests/cases/compiler/getAndSetNotIdenticalType2.ts(9,9): error TS2322: Type 'A<s
            ~
 !!! error TS1056: Accessors are only available when targeting ECMAScript 5 and higher.
            ~
-!!! error TS2380: 'get' and 'set' accessor must have the same type.
+!!! error TS2380: 'get' and 'set' accessor must have the same type 'A<string>'.
+!!! related TS2730 tests/cases/compiler/getAndSetNotIdenticalType2.ts:5:9: The respective 'set' accessor has the type 'A<string>'.


These should not repeat the same type in each message

RyanCavanaugh · 2020-01-08T21:15:16Z

Closing due to lack of activity and outstanding merge conflicts/CI failures.

If anyone's looking at picking this up again, please refer to comments here and on the issue before proceeding - it's not super clear what the best error experience is and it'd be good to establish what a positive outcome looks like first.

bashdx added 3 commits July 3, 2018 21:03

Implementation for issue microsoft#25002

51caec2

Implementation for issue microsoft#25002

2f0cc53

25002 - Resolved merge conflict

f82d295

bashdx mentioned this pull request Jul 3, 2018

Add related error spans for getter/setters with different types #25002

Open

mhegazy reviewed Jul 3, 2018

View reviewed changes

bashdx added 2 commits July 4, 2018 19:40

Resolve merge issue with upstream

9aa2468

microsoft#25002 - changed error message

bf1af7a

mhegazy reviewed Jul 5, 2018

View reviewed changes

microsoft#25002: Implemented changes from feedback

d39e424

DanielRosenwasser requested changes Jul 6, 2018

View reviewed changes

microsoft#25002: removed unneccessary newlines

1d968ab

RyanCavanaugh added this to the TypeScript 3.2 milestone Sep 17, 2018

RyanCavanaugh assigned DanielRosenwasser Sep 17, 2018

DanielRosenwasser modified the milestones: TypeScript 3.2, TypeScript 3.2.1 Nov 13, 2018

DanielRosenwasser modified the milestones: TypeScript 3.2.1, TypeScript 3.3.0 Nov 29, 2018

RyanCavanaugh modified the milestones: TypeScript 3.3, TypeScript 3.4.0 Feb 1, 2019

DanielRosenwasser requested changes Apr 25, 2019

View reviewed changes

RyanCavanaugh modified the milestones: TypeScript 3.4.0, TypeScript 3.6.0 Jun 12, 2019

DanielRosenwasser modified the milestones: TypeScript 3.6.0, TypeScript 3.6.1 Jul 19, 2019

RyanCavanaugh modified the milestones: TypeScript 3.6.1, TypeScript 3.7.0, TypeScript 3.7.1 Sep 25, 2019

RyanCavanaugh closed this Jan 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added implementation for issue #25002 #25415

Added implementation for issue #25002 #25415

bashdx commented Jul 3, 2018

mhegazy Jul 3, 2018

mhegazy Jul 3, 2018

mhegazy Jul 3, 2018

mhegazy Jul 3, 2018

mhegazy Jul 3, 2018

mhegazy commented Jul 3, 2018

bashdx commented Jul 5, 2018

mhegazy Jul 5, 2018

mhegazy Jul 5, 2018

bashdx Jul 5, 2018

mhegazy Jul 5, 2018

bashdx Jul 6, 2018

mhegazy Jul 6, 2018

mhegazy commented Jul 6, 2018

DanielRosenwasser Jul 6, 2018

DanielRosenwasser Jul 6, 2018

mhegazy Jul 6, 2018 •

edited

Loading

bashdx commented Jul 8, 2018

RyanCavanaugh commented Sep 5, 2018

RyanCavanaugh commented Apr 25, 2019

DanielRosenwasser Apr 25, 2019

RyanCavanaugh commented Jan 8, 2020

		const typeNameSecondType = typeToString(secondType);

		if(isGetAccessor(first)){

Added implementation for issue #25002 #25415

Added implementation for issue #25002 #25415

Conversation

bashdx commented Jul 3, 2018

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

mhegazy commented Jul 3, 2018

bashdx commented Jul 5, 2018

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

mhegazy commented Jul 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhegazy Jul 6, 2018 • edited Loading

Choose a reason for hiding this comment

bashdx commented Jul 8, 2018

RyanCavanaugh commented Sep 5, 2018

RyanCavanaugh commented Apr 25, 2019

Choose a reason for hiding this comment

RyanCavanaugh commented Jan 8, 2020

mhegazy Jul 6, 2018 •

edited

Loading