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

Highlight code on diagnostics when underlined #45752

Merged
merged 2 commits into from
Jan 31, 2018

Conversation

estebank
Copy link
Contributor

@estebank estebank commented Nov 4, 2017

Highlight the label's span with the respective color:

Fix #42112.

@rust-highfive
Copy link
Collaborator

r? @pnkfelix

(rust_highfive has picked a reviewer for you, use r? to override)

@estebank
Copy link
Contributor Author

estebank commented Nov 4, 2017

Don't merge yet, needs a quick code change yet.

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 4, 2017
@nikomatsakis
Copy link
Contributor

nikomatsakis commented Nov 4, 2017

@estebank I'm trying to remember why we stopped highlighting the code examples with color, but I cannot. In any case, the examples where there are overlapping spans look...odd to my eye. I wish we had a more elegant way of handling that. I feel like I would prefer if the underlines were drawn on different lines. This is to some extent a distinct issue, but the color makes it more noticeable to me. =)

@durka
Copy link
Contributor

durka commented Nov 4, 2017

What's with the random blue highlighting in some examples? In the first one, // s and the last one, } => {} // seems highlighted for no reason.

@estebank
Copy link
Contributor Author

estebank commented Nov 4, 2017

What's with the random blue highlighting in some examples? In the first one, // s and the last one, } => {} // seems highlighted for no reason.

@durka because of a mistake I made, it's inserting style at position instead of actually replacing, making the output of overlapping spans incorrect, as you see in that screenshot. I didn't notice it until I published the PR :)

I'm trying to remember why we stopped highlighting the code examples with color, but I cannot.

Neither can I :-/

In any case, the examples where there are overlapping spans look...odd to my eye. I wish we had a more elegant way of handling that. I feel like I would prefer if the underlines were drawn on different lines. This is to some extent a distinct issue, but the color makes it more noticeable to me. =)

As for the weirdness around overlapping spans, I feel that 1) it'd get sidestepped by highlighting with bold only (it'd just point towards region without carrying any info about the span type, that'd be only in the underline, so we end up in the same place as today), and 2) even beyond the weirdness of having differently colored code and underline is that it is one way of actually carrying information on where at least one of the spans actually starts without having multiple lines dedicated to the overlapping underlines, but it does look slightly weird. Even if we make the overlapping underlines appear in different lines we're back to really ugly output for thin terminals seen in #42112.

@estebank
Copy link
Contributor Author

estebank commented Nov 5, 2017

Alternative PR #45776 that doesn't use color, only highlights both primary and secondary spans.

@estebank
Copy link
Contributor Author

estebank commented Nov 8, 2017

Moving this to r? @nikomatsakis so that he's the reviewer for both alternative PRs.

@nikomatsakis
Copy link
Contributor

I think there is general consensus that if we were to do anything, it would be #45776, right? Maybe we can close this one?

@estebank
Copy link
Contributor Author

estebank commented Nov 9, 2017

Closing.

@estebank estebank closed this Nov 9, 2017
@nikomatsakis nikomatsakis reopened this Nov 14, 2017
@nikomatsakis
Copy link
Contributor

Based on our recent conversation, maybe this is still on the table?

@estebank
Copy link
Contributor Author

@nikomatsakis yes, let's keep it around.

@carols10cents
Copy link
Member

Triage ping! What's the status of this and #45776? The discussion on users seems to be pretty quiet too, do we need to try and get this more attention?

@estebank
Copy link
Contributor Author

@carols10cents, I think @nikomatsakis and I lean towards merging this one, but I'd like to get some support to disable it in some way before doing so.

@shepmaster shepmaster added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 1, 2017
@shepmaster
Copy link
Member

Sounds like @estebank has some more work to do before we'd be happy merging, yeah?

@nikomatsakis
Copy link
Contributor

@estebank I say let's do it. We can always back off.

@nikomatsakis
Copy link
Contributor

Well, I guess I agree with the idea that we should make it configurable. The time has come, I guess.

@shepmaster
Copy link
Member

@estebank ping from triage — will you have some time to make this configurable?

@estebank
Copy link
Contributor Author

@shepmaster I will try to have something before the end of the year.

@estebank
Copy link
Contributor Author

estebank commented Dec 15, 2017

@nikomatsakis I started playing with using serde and toml to add a $HOME/.rustc configuration file. As it uses serde_derive the compilation fails late in stage 1 failing to find proc_macro, but otherwise it works well. I will try later to see if I can implement the Deserializer for the config. Is it ok to bring these deps into rustc?

You can take a look at the code (broken at the moment) if you want to see the general approach I took.

@nikomatsakis
Copy link
Contributor

Is it ok to bring these deps [serde] into rustc?

cc @alexcrichton @eddyb @rust-lang/compiler -- seems ok to me in principle. I'm not sure if there are technical complications.

@shepmaster shepmaster added the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label Jan 20, 2018
@estebank
Copy link
Contributor Author

@nikomatsakis, rebased and gated it on -Zteach, as I believe it makes sense to do so.

@estebank estebank added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Jan 29, 2018
@nikomatsakis
Copy link
Contributor

good enough for now!

@bors r+

@bors
Copy link
Contributor

bors commented Jan 29, 2018

📌 Commit 08287c1 has been approved by nikomatsakis

@estebank estebank added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 29, 2018
@bors
Copy link
Contributor

bors commented Jan 30, 2018

⌛ Testing commit 08287c1 with merge 9712a3319ba284f218145d17a162b150d896623d...

@bors
Copy link
Contributor

bors commented Jan 30, 2018

💔 Test failed - status-appveyor

@kennytm
Copy link
Member

kennytm commented Jan 30, 2018

@bors retry

3 hour timeout on check-aux x86_64-pc-windows-msvc

@bors
Copy link
Contributor

bors commented Jan 30, 2018

⌛ Testing commit 08287c1 with merge 8ffa119abdc6290be864cad373cd4f7c1a16f76e...

@bors
Copy link
Contributor

bors commented Jan 30, 2018

💔 Test failed - status-appveyor

@kennytm
Copy link
Member

kennytm commented Jan 30, 2018

@bors retry #46903

@bors
Copy link
Contributor

bors commented Jan 30, 2018

⌛ Testing commit 08287c1 with merge ee8dbd216863f3715fbf98ccaf089afa713ae737...

@bors
Copy link
Contributor

bors commented Jan 31, 2018

💔 Test failed - status-appveyor

@estebank
Copy link
Contributor Author

I'll try one more time

@bors retry

@bors
Copy link
Contributor

bors commented Jan 31, 2018

⌛ Testing commit 08287c1 with merge 560a2f4...

bors added a commit that referenced this pull request Jan 31, 2018
Highlight code on diagnostics when underlined

Highlight the label's span with the respective color:

<img width="692" alt="" src="https://user-images.githubusercontent.com/1606434/32411026-a1842482-c18d-11e7-9933-6510eefbad19.png">

Fix #42112.
@bors
Copy link
Contributor

bors commented Jan 31, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 560a2f4 to master...

@bors bors merged commit 08287c1 into rust-lang:master Jan 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.