Put error spans deep on nested object literals #25140
Conversation
|
As a follow-up to today's error sync, let's use |
|
@DanielRosenwasser Done. Also fixed the |
| [30;47m [0m [96m~~~~~~~~~~~~~~~~~~~~~~~~~~~[0m | ||
| [30;47m13[0m } | ||
| [30;47m [0m [96m~~~~~~~~~~~~~[0m | ||
| Which is from property '"another"' of type '{ another: A; }', declared here. |
There was a problem hiding this comment.
Why is "another" in quotes here?
There was a problem hiding this comment.
Because i'm stringifying the literal type associated with the name. It might equally be '0' or [typeof foo]. I can try to strip " from the type's string (if it's a valid identifier post-strip) if you think it'll improve the message.
src/compiler/diagnosticMessages.json
Outdated
| @@ -3710,6 +3710,11 @@ | |||
| "code": 6371 | |||
| }, | |||
|
|
|||
| "Which is from property '{0}' of type '{1}', declared here.": { | |||
There was a problem hiding this comment.
The expected type comes from property '{0}' which was declared here on type '{1}'.
or
The expected type '{0}' comes from property '{1}' which was declared here on type '{2}'.
| return false; | ||
| } | ||
|
|
||
| function elaborateError(node: Expression | undefined, source: Type, target: Type): boolean { |
There was a problem hiding this comment.
shouldElaborateErrorOnProperty
There was a problem hiding this comment.
It's not a check - it actually performs the elaboration and adds the new diagnostics - its return type merely indicates weather such an elaboration occurred.
src/compiler/checker.ts
Outdated
| return false; | ||
| } | ||
|
|
||
| function *generateObjectliteralElements(node: ObjectLiteralExpression): ElaborationIterator { |
src/compiler/diagnosticMessages.json
Outdated
| @@ -3710,6 +3710,11 @@ | |||
| "code": 6371 | |||
| }, | |||
|
|
|||
| "Which is from property '{0}' of type '{1}', declared here.": { | |||
| "category": "Message", | |||
There was a problem hiding this comment.
I thought it was "category": "Error"
There was a problem hiding this comment.
As discussed duing our sync earlier, right now the category is unused for related information - we can put off deciding why they aughta be until we have a story for using them in some principled way (for, eg, elaborations vs explanations)
src/compiler/utilities.ts
Outdated
| * | ||
| * @return non-quoted string | ||
| */ | ||
| export function stripQuotes(name: string) { |
There was a problem hiding this comment.
I feel like we should just be able to call symbolToString and get the right thing in certain cases, and just funnel along a string.
|
@DanielRosenwasser I've synced with master, hopefully addressed your reviews, and added a few more tests for good measure. Have anything more you'd like to see? |
src/compiler/checker.ts
Outdated
| if (!length(node.properties)) return; | ||
| for (const prop of node.properties) { | ||
| if (isJsxSpreadAttribute(prop)) continue; | ||
| yield [prop.name, prop.initializer, getLiteralType(idText(prop.name))]; |
There was a problem hiding this comment.
It's hard to tell what the intent is here; I'd make this an object instead of a tuple.
src/compiler/checker.ts
Outdated
| // Assignability failure - check each prop individually, and if that fails, fall back on the bad error span | ||
| let reportedError = false; | ||
| for (let status = iterator.next(); !status.done; status = iterator.next()) { | ||
| const [prop, next, nameType] = status.value; |
There was a problem hiding this comment.
Maybe make it clear that you're diving into next; it doesn't seem immediately obvious.
| if (!isTypeAssignableTo(sourcePropType, targetPropType)) { | ||
| const elaborated = next && elaborateError(next, sourcePropType, targetPropType); | ||
| if (elaborated) { | ||
| reportedError = true; |
There was a problem hiding this comment.
I think this can just become a return
There was a problem hiding this comment.
No, if I were to return here, i would only issue an elaboration on the first property with an issue instead of all of them.
There was a problem hiding this comment.
I could continue and un-indent and remove the else if you'd prefer that, however.
src/compiler/checker.ts
Outdated
| } | ||
|
|
||
| type ElaborationIterator = IterableIterator<[Node, Expression | undefined, Type]>; | ||
| function elaborateElementwise(iterator: ElaborationIterator, source: Type, target: Type) { |
There was a problem hiding this comment.
Can you write a quick bit about what the intuition should be before I dive into this function?
src/compiler/checker.ts
Outdated
| return checkTypeRelatedTo(source, target, assignableRelation, errorNode, headMessage, containingMessageChain, errorOutputObject); | ||
| } | ||
|
|
||
| function checkTypeAssignableToAndOptionallyElaborate(source: Type, target: Type, errorNode: Node | undefined, expr: Expression | undefined, headMessage?: DiagnosticMessage, containingMessageChain?: () => DiagnosticMessageChain | undefined): boolean { |
There was a problem hiding this comment.
Add a little blurb that this performs a structural comparison using a given expression that we can use to provide specific errors on things like properties.
Ahhh.... to what are you referring? |
|
Looks like you do have a case - however, as we discussed offline, the computed property message is a little confusing given that it's not clear it you're erroring on the name. |
|
@DanielRosenwasser Added; what do you think about the new error? |
src/compiler/diagnosticMessages.json
Outdated
| @@ -1452,6 +1452,10 @@ | |||
| "category": "Error", | |||
| "code": 2417 | |||
| }, | |||
| "Type of computed property value is '{0}', which is not assignable to type '{1}'.": { | |||
And include an additional location pointing back at the deeply nested property in the target type it is being assigned to. Additionally, like in classes which are incompatible with a base type, object and array literals will now flag all locations which are incompatible with the target instead of just one.
Fixes #25030 partially (for all instances involving expressions where we're tracking which properties have issues).