Use smaller def span for functions #75465

Aaron1011 · 2020-08-12T21:20:01Z

Currently, the def span of a function encompasses the entire function
signature and body. However, this is usually unnecessarily verbose - when we are
pointing at an entire function in a diagnostic, we almost always want to
point at the signature. The actual contents of the body tends to be
irrelevant to the diagnostic we are emitting, and just takes up
additional screen space.

This commit changes the def_span of all function items (freestanding
functions, impl-block methods, and trait-block methods) to be the
span of the signature. For example, the function

pub fn foo<T>(val: T) -> T { val }

now has a def_span corresponding to pub fn foo<T>(val: T) -> T
(everything before the opening curly brace).

Trait methods without a body have a def_span which includes the
trailing semicolon. For example:

trait Foo {
    fn bar();
}

the function definition Foo::bar has a def_span of fn bar();

This makes our diagnostic output much shorter, and emphasizes
information that is relevant to whatever diagnostic we are reporting.

We continue to use the full span (including the body) in a few of
places:

MIR building uses the full span when building source scopes.
'Outlives suggestions' use the full span to sort the diagnostics being
emitted.
The #[rustc_on_unimplemented(enclosing_scope="in this scope")]
attribute points the entire scope body.

All of these cases work only with local items, so we don't need to
add anything extra to crate metadata.

rust-highfive · 2020-08-12T21:20:05Z

r? @lcnr

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

Aaron1011 · 2020-08-12T22:56:52Z

src/test/ui/type-alias-impl-trait/generic_duplicate_param_use6.stderr

-LL | | }
-   | |_^ expected `(T, T)`, got `(U, T)`
+LL | fn three<T: Copy + Debug, U: Debug>(t: T, u: U) -> Two<T, U> {
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `(T, T)`, got `(U, T)`


This seems slightly worse than the previous message. However, I think this should really be pointing to a value being returned (either an explicit return or the trailing expression). This is a nightly feature, so I think it's fine to improve this later (e.g. before stabilization) as needed.

I think we probably should do so in general, but that isn't bad enough for me to block this pr

Hard agree on all points.

lcnr

I really like these changes 👍

cc @estebank I would expect that you are also interested in this.

lcnr · 2020-08-13T13:18:26Z

src/test/ui/async-await/multiple-lifetimes/ret-impl-trait-no-fg.stderr

-LL | |
-...  |
-LL | |     (a, b)
-LL | | }


For impl Trait it would be helpful to see the return expression here

This seems slightly worse than the previous message. However, I think this should really be pointing to a value being returned (either an explicit return or the trailing expression). This is a nightly feature, so I think it's fine to improve this later (e.g. before stabilization) as needed.

pretty much this

lcnr · 2020-08-13T13:30:57Z

src/test/ui/issues/issue-67552.stderr

@@ -10,11 +10,7 @@ note: `rec` defined here
 LL | / fn rec<T>(mut it: T)
 LL | | where
 LL | |     T: Iterator,


I personally might even prefer to exclude where bounds from the function signature 🤔

so we only point at fn rec<T>(mut it: T) here

I think it's better to display the where clause, so switching from fn<T: Copy>() {} to fn<T>() where T: Copy {} doesn't change the diagnostic output.

I don't fully get what you mean here, wouldn't that change change the diagnostic output from fn<T: Copy>() to fn<T>() where T: Copy with this PR?

I think that diagnostics where where bounds are relevant mention the where bound itself, so I think that not mentioning them in the signature is worth it as it can remove noise.

Sorry - I meant that if a user switches from an 'inline' <T: Copy> to a 'where-clause' where T: Copy, they should still see the same amount of information in diagnostics.

hmm, I am not sure how often these bounds help and are not explicitly references and put a fairly high value into single line error spans.

But that's just a personal preference and I am fine with landing this as is, so r=me. We may want to wait a day or two in case @estebank want's to look over this but otherwise I am satisfied

I think that the safe approach will be to include them.

lcnr · 2020-08-13T13:44:42Z

src/test/ui/type-alias-impl-trait/generic_duplicate_param_use6.stderr

-LL | | }
-   | |_^ expected `(T, T)`, got `(U, T)`
+LL | fn three<T: Copy + Debug, U: Debug>(t: T, u: U) -> Two<T, U> {
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `(T, T)`, got `(U, T)`


I think we probably should do so in general, but that isn't bad enough for me to block this pr

Aaron1011 · 2020-08-21T20:44:52Z

@estebank: Do you have any objections to this change?

estebank

+100 from me. Left a couple of nitpicks. We need to be carefully going forward to not use this new span when we actually want to point at the body, but it should be fine.

src/librustc_middle/hir/map/mod.rs

estebank · 2020-08-21T23:34:03Z

src/librustc_parse/parser/item.rs

-        let body = self.parse_fn_body(attrs)?; // `;` or `{ ... }`.
-        Ok((ident, FnSig { header, decl }, generics, body))
+
+        let mut sig_hi = self.prev_token.span;


Suggested change

let mut sig_hi = self.prev_token.span;

let sig_hi = if self.check(&token::Semi) {

// Include the trailing semicolon in the span of the signature

self.token.span

} else {

self.prev_token.span

};

This will introduce a duplicate check for token::Semi - I wanted to make sure that the determination of sig_hi always stays in sync with the actual parsing logic.

ok, fair enough it's not critical

estebank · 2020-08-21T23:34:16Z

src/librustc_parse/parser/item.rs

+            // Include the trailing semicolon in the span of the signature
+            *sig_hi = self.token.span;


Suggested change

// Include the trailing semicolon in the span of the signature

*sig_hi = self.token.span;

estebank · 2020-08-21T23:34:37Z

src/librustc_parse/parser/item.rs

+    fn parse_fn_body(
+        &mut self,
+        attrs: &mut Vec<Attribute>,
+        sig_hi: &mut Span,


Suggested change

sig_hi: &mut Span,

Same issue as above

estebank · 2020-08-21T23:36:40Z

src/test/ui/issues/issue-67552.stderr

@@ -10,11 +10,7 @@ note: `rec` defined here
 LL | / fn rec<T>(mut it: T)
 LL | | where
 LL | |     T: Iterator,


I think that the safe approach will be to include them.

estebank · 2020-08-21T23:37:11Z

src/test/ui/regions/regions-trait-1.stderr

+LL |     fn get_ctxt(&self) -> &'a Ctxt {
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^


This change is slightly unfortunate given the message.

Maybe it should only highlight the part from { up to and including } and not the function signature itself?

estebank · 2020-08-21T23:37:50Z

src/test/ui/type-alias-impl-trait/generic_duplicate_param_use6.stderr

-LL | | }
-   | |_^ expected `(T, T)`, got `(U, T)`
+LL | fn three<T: Copy + Debug, U: Debug>(t: T, u: U) -> Two<T, U> {
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `(T, T)`, got `(U, T)`


Hard agree on all points.

estebank · 2020-08-21T23:39:17Z

(Sorry about the delay in review, I've been having trouble staying on top of my notification volume lately.)

Aaron1011 · 2020-08-22T17:02:56Z

@estebank: I've addressed your comments. My concern with removing sig_hi: &mut Span is that computation of the def span could eventually get out of sync with the tokens that we actually consume.

estebank · 2020-08-22T19:54:25Z

r? @estebank @bors r+

bors · 2020-08-22T19:54:26Z

📌 Commit 8b886a32350739abe63a0f57cac0a7be0092b737 has been approved by estebank

bors · 2020-08-22T20:40:56Z

⌛ Testing commit 8b886a32350739abe63a0f57cac0a7be0092b737 with merge f1e1f6d98a3a41673551f10c462a0c72be3db53e...

bors · 2020-08-22T21:12:48Z

💔 Test failed - checks-actions

Currently, the def span of a funtion encompasses the entire function signature and body. However, this is usually unnecessarily verbose - when we are pointing at an entire function in a diagnostic, we almost always want to point at the signature. The actual contents of the body tends to be irrelevant to the diagnostic we are emitting, and just takes up additional screen space. This commit changes the `def_span` of all function items (freestanding functions, `impl`-block methods, and `trait`-block methods) to be the span of the signature. For example, the function ```rust pub fn foo<T>(val: T) -> T { val } ``` now has a `def_span` corresponding to `pub fn foo<T>(val: T) -> T` (everything before the opening curly brace). Trait methods without a body have a `def_span` which includes the trailing semicolon. For example: ```rust trait Foo { fn bar(); }``` the function definition `Foo::bar` has a `def_span` of `fn bar();` This makes our diagnostic output much shorter, and emphasizes information that is relevant to whatever diagnostic we are reporting. We continue to use the full span (including the body) in a few of places: * MIR building uses the full span when building source scopes. * 'Outlives suggestions' use the full span to sort the diagnostics being emitted. * The `#[rustc_on_unimplemented(enclosing_scope="in this scope")]` attribute points the entire scope body. * The 'unconditional recursion' lint uses the full span to show additional context for the recursive call. All of these cases work only with local items, so we don't need to add anything extra to crate metadata.

Aaron1011 · 2020-08-22T22:43:57Z

I needed to bless the NLL tests.

@estebank: I've squashed the commits, and updated the UNCONDITIONAL_RECURSION lint to use the full body span. This preserves the current lint output, which shows useful additional context around the recursive call.

estebank · 2020-08-22T23:19:07Z

@bors r+

bors · 2020-08-22T23:19:08Z

📌 Commit e3cd43e has been approved by estebank

bors · 2020-08-23T08:14:22Z

⌛ Testing commit e3cd43e with merge d5ba3ef...

bors · 2020-08-23T10:25:45Z

☀️ Test successful - checks-actions, checks-azure
Approved by: estebank
Pushing d5ba3ef to master...

This was missed in PR rust-lang#75465. As a result, a few places have been using the full body span of functions, instead of just the header span.

Record `tcx.def_span` instead of `item.span` in crate metadata This was missed in PR rust-lang#75465. As a result, a few places have been using the full body span of functions, instead of just the header span.

This was missed in PR rust-lang#75465. As a result, a few places have been using the full body span of functions, instead of just the header span.

Record `tcx.def_span` instead of `item.span` in crate metadata This was missed in PR rust-lang#75465. As a result, a few places have been using the full body span of functions, instead of just the header span.

Currently, we serialize the same crate metadata for proc-macro crates as we do for normal crates. This is quite wasteful - almost none of this metadata is ever used, and much of it can't even be deserialized (if it contains a foreign `CrateNum`). This PR changes metadata encoding to skip encoding the majority of crate metadata for proc-macro crates. Most of the `Lazy<[T]>` fields are left completetly empty, while the non-lazy fields are left as-is. Additionally, proc-macros now have a def span that does not include their body. This was done for normal functions in rust-lang#75465, but was missed for proc-macros. As a result of this PR, we should only ever encode local `CrateNum`s when encoding proc-macro crates. I've added a specialized serialization impl for `CrateNum` to assert this.

…data, r=petrochenkov Encode less metadata for proc-macro crates Currently, we serialize the same crate metadata for proc-macro crates as we do for normal crates. This is quite wasteful - almost none of this metadata is ever used, and much of it can't even be deserialized (if it contains a foreign `CrateNum`). This PR changes metadata encoding to skip encoding the majority of crate metadata for proc-macro crates. Most of the `Lazy<[T]>` fields are left completetly empty, while the non-lazy fields are left as-is. Additionally, proc-macros now have a def span that does not include their body. This was done for normal functions in rust-lang#75465, but was missed for proc-macros. As a result of this PR, we should only ever encode local `CrateNum`s when encoding proc-macro crates. I've added a specialized serialization impl for `CrateNum` to assert this.

rust-highfive assigned lcnr Aug 12, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 12, 2020

Aaron1011 commented Aug 12, 2020

View reviewed changes

lcnr approved these changes Aug 13, 2020

View reviewed changes

Aaron1011 mentioned this pull request Aug 17, 2020

Add CONST_ITEM_MUTATION lint #75573

Merged

estebank approved these changes Aug 21, 2020

View reviewed changes

Aaron1011 force-pushed the feature/short-fn-def-span branch from 2ad6322 to 483540d Compare August 22, 2020 16:59

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-review Status: Awaiting review from the assignee but also interested parties. labels Aug 22, 2020

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 Aug 22, 2020

Aaron1011 force-pushed the feature/short-fn-def-span branch from 8b886a3 to e3cd43e Compare August 22, 2020 22:42

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-review Status: Awaiting review from the assignee but also interested parties. labels Aug 22, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 23, 2020

bors merged commit d5ba3ef into rust-lang:master Aug 23, 2020

This was referenced Aug 23, 2020

diagnostics: shorten paths of unique symbols #73996

Merged

ensure arguments are included in count mismatch span #75023

Merged

Aaron1011 mentioned this pull request Sep 19, 2020

Encode less metadata for proc-macro crates #76897

Merged

Aaron1011 mentioned this pull request Sep 19, 2020

Record tcx.def_span instead of item.span in crate metadata #76898

Merged

cuviper added this to the 1.47.0 milestone May 2, 2024

		// Include the trailing semicolon in the span of the signature
		*sig_hi = self.token.span;

		LL \| fn get_ctxt(&self) -> &'a Ctxt {
		\| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Use smaller def span for functions #75465

Use smaller def span for functions #75465

Conversation

Aaron1011 commented Aug 12, 2020 • edited Loading

rust-highfive commented Aug 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcnr left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aaron1011 Aug 13, 2020 • edited Loading

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

Aaron1011 commented Aug 21, 2020

estebank left a 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

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

estebank commented Aug 21, 2020

Aaron1011 commented Aug 22, 2020

estebank commented Aug 22, 2020

bors commented Aug 22, 2020

bors commented Aug 22, 2020

bors commented Aug 22, 2020

Aaron1011 commented Aug 22, 2020

estebank commented Aug 22, 2020

bors commented Aug 22, 2020

bors commented Aug 23, 2020

bors commented Aug 23, 2020

Aaron1011 commented Aug 12, 2020 •

edited

Loading

Aaron1011 Aug 13, 2020 •

edited

Loading