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

Add -Zteach documentation #47843

Merged
merged 2 commits into from
Feb 12, 2018
Merged

Add -Zteach documentation #47843

merged 2 commits into from
Feb 12, 2018

Conversation

estebank
Copy link
Contributor

Add extra inline documentation to E0019, E0016, E0013, E0396, E0017,
E0018, E0010, E0022, E0030, E0029, E0033, E0026 and E0027.

Follow up to #47652.

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

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

@estebank
Copy link
Contributor Author

r? @nikomatsakis

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 29, 2018
@Manishearth
Copy link
Member

Got a list or things that need Zteach? I'd like to help. I did a large fraction of the extended messages and wished something like this existed back then.

(Have some ideas of my own for Zteach which i may try later)

@estebank
Copy link
Contributor Author

@Manishearth No list at the moment, I'm just going through the error index looking for explanations that do not need changes to the current diagnostics engine just to get coverage. I think there're are only 3 errors covered at the moment. From my high level look, about 1/3 of all errors do not need any improvement, about 1/3 just need notes added to them and the remaining 1/3 require changes to the diagnostic engine to make it possible to provide sample code, labels on subdiagnostics, etc.

I would like to focus on the easy one firsts and get the ball rolling. I'll create an issue with a copy of the index so we can check them out as we add them, separated by difficulty/urgency. Does that sound reasonable?

if self.tcx.sess.teach(&err.get_code().unwrap()) {
err.note(
"The value of statics and constants must be known at compile time, \
and they live for the entire lifetime of a program. Creating a boxed \
Copy link
Contributor

Choose a reason for hiding this comment

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

does the note code word wrap?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't at the moment.

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

I feel like we should have some -Zteach UI test cases for these -- or at least some of them, no?

Perhaps we can somehow automate this?

@nikomatsakis
Copy link
Contributor

I'm a bit torn here. I like the idea of picking the low-hanging fruit, but I worry that we are going to give people the wrong idea on the overall direction here by hand-coding all the things into the code. I guess I'd be happier if we had some place -- be it an RFC, tracking issue, or internals thread -- where we were discussing the overall direction and plans with -Zteach.

At the moment, I tend to think we ought to be working on getting the error strings out of the code, not moving them in. I'd rather see general templates being used. (But I worry maybe it won't give us the flexibility we need.)

@Manishearth
Copy link
Member

When I was working on the extended error codes we eventually built an issue where we tracked all of the errors.

A templating system where you pass something like a hashmap of strings to a template would be nice.

@nikomatsakis
Copy link
Contributor

@Manishearth

A templating system where you pass something like a hashmap of strings to a template would be nice.

I envisioned roughly this, except that the parameters would come in two forms:

  • displayable things (like strings, types, etc)
  • spans

The spans would be used when constructing snippets or showing code.

There are some gaps to fill though, like suggestions, which often require some custom code; but I guess that could be another sort of parameter to be passed in?

@estebank estebank 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 Jan 30, 2018
@steveklabnik
Copy link
Member

i'd like to shout-out to https://crates.io/crates/handlebars if you're looking for templating, it is exactly

A templating system where you pass something like a hashmap of strings to a template would be nice.

@nikomatsakis
Copy link
Contributor

Indeed, I had hoped to use handlebars or some such thing, or perhaps an extended version of it (to support the span snippets)

@estebank
Copy link
Contributor Author

I'm slightly intrigued by how you envision a given template string to look like.

Would it be something like the following?:

{error message}
some descriptive text
X | {span} span label content
note: a note associated to the above diagnostic

note: some note content associated to he following subdiagnostic
X | {other_span} other span label content
X | {extra_span} extra span label content

help: some help text

suggestion: suggestion label
X | {suggestion_span}->{suggestion_content}

teach help: some extended content that will only be printed out when passing `-Zteach`

@GuillaumeGomez
Copy link
Member

That's awesome!

@nikomatsakis
Copy link
Contributor

@estebank

I'm slightly intrigued by how you envision a given template string to look like.

I was hoping we could go so far as to unify everything into a template (including the short-errors). In that case, let me pick a specific sample.

Let's say this borrowck error:

error[E0503]: cannot use `x` because it was mutably borrowed
 --> src/main.rs:4:9
  |
3 |     let y = &mut x;
  |                  - borrow of `x` occurs here
4 |     let z = x;
  |         ^ use of borrowed `x`

I imagine the inputs might be something like:

borrowed_path: &impl Display,
borrow_span: Span
use_span: Span

and hence the invocation might be like:

self.tcx.report(Errors::E0593 {
    borrowed_path: &borrowed_path,
    borrow_span: the_borrow_span,
    use_span: the_use_span,
});

And then the "short template" might be like:

error: cannot use `{{borrowed_path}}` because it was mutably borrowed
{{snippet
    primary borrow_span
    label borrow_span "borrow of `{{borrowed_path}}` occurs here"
    label use_span "use of borrowed `{{borrowed_path}}`"}}

(I have no idea if that's even remotely valid Handlebars syntax, but hopefully you get the idea.)

The --teach variant might be:

error: cannot use `{{borrowed_path}}` because it was mutably borrowed

Rust's mutability rules state that when a value is mutably borrowed with an `&mut` expression, that value can **only** be accessed through the resulting reference. In your code, an `&mut` borrow occurs here:

{{snippet primary borrow_span}}

But then the code later attempts to use `{{borrowed_path}}` here:

{{snippet primary use_span}}

(Note: not suggesting this is a particular good --teach error =)

Obviously this format will be a bit tricky. Among other things, there are often "subtle variants" on how messages are presented; for a single error code, we may need the ability to have variants, e.g., E0593_1 and so forth. But this will be needed for translation at some point anyway. And naturally there are optional hints and suggestions; not sure how those would best fit into this scheme.

(Also, we probably want to make the "short message" syntax a bit more descriptive, e.g. by just specifying the 'main message' and the labels, so that we can more readily make "mass changes" to the format.)

@nikomatsakis
Copy link
Contributor

Er, I forgot. r=me if we add some a ui test or two =)

Add extra inline documentation to E0019, E0016, E0013, E0396, E0017,
E0018, E0010, E0022, E0030, E0029, E0033, E0026 and E0027.
@estebank
Copy link
Contributor Author

estebank commented Feb 8, 2018

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Feb 8, 2018

📌 Commit 6e8281b has been approved by nikomatsakis

@bors bors 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 8, 2018
@bors
Copy link
Contributor

bors commented Feb 8, 2018

⌛ Testing commit 6e8281bf7427c4b7ef8f62bb9c41a69dc0b6ab50 with merge b816f42db48152433ac28bc6b03299bda04e131b...

@bors
Copy link
Contributor

bors commented Feb 8, 2018

💔 Test failed - status-appveyor

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

kennytm commented Feb 8, 2018

Legit, the E0583 UI test failed on Windows.

The $DIR replacement will happen only if the path uses backslashes.

[01:14:05] failures:
[01:14:05] 
[01:14:05] ---- [ui] ui\error-codes\E0583.rs stdout ----
[01:14:05] 	diff of stderr:
[01:14:05] 
[01:14:05] 4	11 | mod module_that_doesnt_exist; //~ ERROR E0583
[01:14:05] 5	   |     ^^^^^^^^^^^^^^^^^^^^^^^^
[01:14:05] 6	   |
[01:14:05] -	   = help: name the file either module_that_doesnt_exist.rs or module_that_doesnt_exist/mod.rs inside the directory "$DIR"
[01:14:05] +	   = help: name the file either module_that_doesnt_exist.rs or module_that_doesnt_exist/mod.rs inside the directory "C:/projects/rust/src/test/ui/error-codes"
[01:14:05] 8	
[01:14:05] 9	error: aborting due to previous error
[01:14:05] 10	

@kennytm kennytm 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 Feb 8, 2018
@estebank
Copy link
Contributor Author

estebank commented Feb 8, 2018

@bors r=nikomatsakis

@kennytm I moved that test back to compile-fail. I'll get back to it later.

@bors
Copy link
Contributor

bors commented Feb 8, 2018

📌 Commit 51f0c0d has been approved by nikomatsakis

@bors bors 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 8, 2018
@bors
Copy link
Contributor

bors commented Feb 12, 2018

⌛ Testing commit 51f0c0d with merge 16362c7...

bors added a commit that referenced this pull request Feb 12, 2018
Add `-Zteach` documentation

Add extra inline documentation to E0019, E0016, E0013, E0396, E0017,
E0018, E0010, E0022, E0030, E0029, E0033, E0026 and E0027.

Follow up to #47652.
@bors
Copy link
Contributor

bors commented Feb 12, 2018

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

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.

9 participants