GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Introduce related spans into tsserver protocol by weswigham · Pull Request #24548 · microsoft/TypeScript

Conversation

Wesley Wigham (weswigham) commented Jun 1, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

This just introduces related information spans into our protocol, this way we can start adding them to diagnostics and seeing them light up in the editor - needs a small change in vscode once the protocol element has been published as well.

Ref #22789, #10489

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Introduce related spans into tsserver protocol

e238490

Wesley Wigham (weswigham) requested review from a user, Daniel Rosenwasser (DanielRosenwasser), Andrew Casey (amcasey) and Mohamed Hegazy (mhegazy)

June 1, 2018 00:11

Wesley Wigham (weswigham) mentioned this pull request

Jun 1, 2018

JSX errors use JSX terminology #24523

Closed

Andrew Casey (amcasey) reviewed

Jun 2, 2018

View reviewed changes

Comment thread

src/compiler/types.ts

		file: SourceFile \| undefined;
		start: number \| undefined;
		length: number \| undefined;
		messageText: string \| DiagnosticMessageChain;

Andrew Casey (amcasey) Jun 2, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

messageText: string | DiagnosticMessageChain; [](start = 8, length = 45)

I think I need more information about how these are intended to be used. Roslyn diagnostics support additional locations, but those locations don't have messages. We could create separate diagnostics but we would need a new code and category and I don't believe we'd be able to link them together. I think Roslyn more or less dropped this functionality (vs the old native compiler).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Wesley Wigham (weswigham) Jun 2, 2018

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Secondary code locations and more information pertaining to specifically that location in the context of the diagnostic. (eg, "Defined here", "Declared here", "Replicated here", and so on. The additional spans also allow us to use those spans when calculating quick fixes - so we can, for example, know what we can suggest a cast at the site of an assignability error, but also suggest modifying the type the assignment is failing against (because it is mentioned as a related span). The only diagnostic set we currently do a similar thing with is the esModuleInterop import error, which we currently issue a secondary diagnostic for (so there's no obvious link between the two).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Wesley Wigham (weswigham) Jun 2, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reference, this isn't a new concept and there is prior art, which is why vscode has basic support for them (and is looking to iterate on their UI) - rust makes good use of related spans in it's errors.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Andrew Casey (amcasey) Jun 4, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not saying that it's either novel or bad, just that it goes against the grain of the API we're using.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mohamed Hegazy (mhegazy) Jun 5, 2018

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to sit down and design this feature.. there seems to be two uses cases,

additional locations, e.g. duplicate identifier errors, you would list them all in the same error message
related diagnostics, e.g. type of var x is not assignable to string, and here is where x was declated

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Andrew Casey (amcasey) reviewed

Jun 2, 2018

View reviewed changes

Comment thread

src/harness/unittests/tsserverProjectSystem.ts

		function createDiagnostic(start: protocol.Location, end: protocol.Location, message: DiagnosticMessage, args: ReadonlyArray<string> = [], category = diagnosticCategoryName(message), reportsUnnecessary?: {}): protocol.Diagnostic {
		return { start, end, text: formatStringFromArgs(message.message, args), code: message.code, category, reportsUnnecessary, source: undefined };
		function createDiagnostic(start: protocol.Location, end: protocol.Location, message: DiagnosticMessage, args: ReadonlyArray<string> = [], category = diagnosticCategoryName(message), reportsUnnecessary?: {}, relatedInformation?: protocol.DiagnosticRelatedInformation[]): protocol.Diagnostic {
		return { start, end, text: formatStringFromArgs(message.message, args), code: message.code, category, reportsUnnecessary, relatedInformation, source: undefined };

Andrew Casey (amcasey) Jun 2, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

relatedInformation [](start = 130, length = 18)

Should this new property be validated in some way?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Wesley Wigham (weswigham) Jun 2, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing has been modified to produce one yet, though we have a massive list courtesy of Daniel Rosenwasser (@DanielRosenwasser) - this just makes it so we can start chipping that list away.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

amcasey reacted with thumbs up emoji

All reactions

👍 1 reaction

Mohamed Hegazy (mhegazy) reviewed

Jun 5, 2018

View reviewed changes

Comment thread

src/server/session.ts

		relatedInformation: map(diag.relatedInformation, formatRelatedInformation),
		};
		return includeFileName
		? { ...common, fileName: diag.file && diag.file.fileName }

Mohamed Hegazy (mhegazy) Jun 5, 2018

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to create an object just to inline it.. you can set fileName to undefined

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mohamed Hegazy (mhegazy) approved these changes

Jun 7, 2018

View reviewed changes

Mohamed Hegazy (mhegazy) commented Jun 7, 2018

Copy link

Copy Markdown

Contributor

//CC Matt Bierner (@mjbvz)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Matt Bierner (mjbvz) approved these changes

Jun 7, 2018

View reviewed changes

Mohamed Hegazy (mhegazy) commented Jun 7, 2018

Copy link

Copy Markdown

Contributor

Wesley Wigham (@weswigham) we should also show the additional locations under --pretty.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          Merge branch 'master' into allow-relatedinfo-on-diag

fb03d91

Wesley Wigham (weswigham) force-pushed the allow-relatedinfo-on-diag branch from a51811c to fb03d91 Compare

June 14, 2018 21:43

Wesley Wigham (weswigham) commented Jun 14, 2018

Copy link

Copy Markdown

Member Author

Mohamed Hegazy (@mhegazy)

thoughts?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          Incorporate pretty output, implement esModuleInterop related span

2251da1

Wesley Wigham (weswigham) commented Jun 15, 2018

Copy link

Copy Markdown

Member Author

Mohamed Hegazy (@mhegazy) I've implemented the pretty output (as above) as well as the esModuleInterop import error related information.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Wesley Wigham (weswigham) merged commit 640af3f into microsoft:master

Jun 15, 2018

Wesley Wigham (weswigham) deleted the allow-relatedinfo-on-diag branch

June 15, 2018 17:54

Wesley Wigham (weswigham) commented Jun 15, 2018

Copy link

Copy Markdown

Member Author

Daniel Rosenwasser (@DanielRosenwasser) Wanna open your massive list of error-updating backlog issues now that this is in?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This was referenced Jun 15, 2018

Open

Closed

Open

Daniel Rosenwasser (DanielRosenwasser) mentioned this pull request

Jun 26, 2018

Closed

Daniel Rosenwasser (DanielRosenwasser) added the Domain: Related Error Spans Specifying regions for error messages/diagnostics on multiple locations. label

Jun 26, 2018

Daniel Rosenwasser (DanielRosenwasser) mentioned this pull request

Jun 27, 2018

TypeScript 3.0 related error spans meta issue #25257

Closed

5 tasks done

Microsoft (microsoft) locked as resolved and limited conversation to collaborators

Oct 21, 2025

Reviewers

Daniel Rosenwasser (DanielRosenwasser)

Awaiting requested review from DanielRosenwasser

3 more reviewers

Andrew Casey (amcasey)

amcasey left review comments

Mohamed Hegazy (mhegazy)

mhegazy approved these changes

Matt Bierner (mjbvz)

mjbvz approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

Domain: Related Error Spans

Specifying regions for error messages/diagnostics on multiple locations.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

Wesley Wigham (weswigham) commented Jun 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Andrew Casey (amcasey) Jun 2, 2018

Choose a reason for hiding this comment

Uh oh!

Wesley Wigham (weswigham) Jun 2, 2018

Choose a reason for hiding this comment

Uh oh!

Wesley Wigham (weswigham) Jun 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Andrew Casey (amcasey) Jun 4, 2018

Choose a reason for hiding this comment

Uh oh!

Mohamed Hegazy (mhegazy) Jun 5, 2018

Choose a reason for hiding this comment

Uh oh!

Andrew Casey (amcasey) Jun 2, 2018

Choose a reason for hiding this comment

Uh oh!

Wesley Wigham (weswigham) Jun 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mohamed Hegazy (mhegazy) Jun 5, 2018

Choose a reason for hiding this comment

Uh oh!

Mohamed Hegazy (mhegazy) commented Jun 7, 2018

Uh oh!

Mohamed Hegazy (mhegazy) commented Jun 7, 2018

Uh oh!

Wesley Wigham (weswigham) commented Jun 14, 2018

Uh oh!

Wesley Wigham (weswigham) commented Jun 15, 2018

Uh oh!

Wesley Wigham (weswigham) commented Jun 15, 2018

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Wesley Wigham (weswigham) commented Jun 1, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Wesley Wigham (weswigham) Jun 2, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Wesley Wigham (weswigham) Jun 2, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.