Add Span field for Generics structs #35591
Conversation
|
LGTM @bors r+ |
|
📌 Commit 652f953 has been approved by |
|
Plugin breaking, cc @Manishearth |
|
@bors r- Is this something that needs to merge now, or can it wait? |
|
@Manishearth - do you mean wait until the merge to beta? I was chatting with @nikomatsakis this morning about spans in the AST. His feeling (though I'll let him chime in) was generally that spans that help us give more accurate error messages in the hir/ast were okay. |
|
No, wait until the next libsyntax breaking batch in #31645. That will happen when we have a few more breaking PRs or a breaking PR that needs to merge because it blocks something. |
|
@Manishearth - when is that? There's a huge batch of error improvements going in for 1.12. Would be great to land some of these improved spans, also, rather than having to wait a month. Maybe we can collect a few together and make that a batch? |
|
It's not obvious from this PR what the benefit is, but it definitely adds a lot of bytes to the AST (and the metadata?). There is one test case here and I don't actually see how the change affects it. |
|
@brson - this change is relatively minor but there are other people working on adding spans to the AST. The idea for the group of changes is to better be able to underline a span much closer to the source of problems. For example, if the generic part of the type is wrong, underline just that. Same for, say, an unused function. If you can underline something like the function name, that will generally read better than underlining the whole function (or just the first char, which is what we do now) Again, one by one they're smallish improvements. This one iirc was @GuillaumeGomez trying to address the feedback from a previous pr comment that the span for generics wasn't accurate enough. But, I think taken in aggregate, the little improvements will add up. Thousands of people saving a second here and there. |
|
So, yeah, of this is part of a larger refactor I'd prefer for that refactoring to be merged in one go over small bits to minimize aster breakage (which makes the serde-using ecosystem hiccup). |
|
@Manishearth roger that. Cc @nikomatsakis |
|
It doesn't have to be all in one chunk, so if it gets too unweildy at some point or very bitrot-y ping me and I'll initiate the merge process(giving heads ups and stuff before r+) |
| fld: &mut T) -> Generics { | ||
| Generics { | ||
| ty_params: fld.fold_ty_params(ty_params), | ||
| lifetimes: fld.fold_lifetime_defs(lifetimes), | ||
| where_clause: fld.fold_where_clause(where_clause), | ||
| span: span, |
There was a problem hiding this comment.
All spans should be folded with new_span; that is, this should be span: fld.new_span(span),.
There was a problem hiding this comment.
It gets a bit ugly but as you prefer. ;)
GuillaumeGomez
force-pushed
the
generics_span
branch
from
652f953
to
5d45ad0
Compare
|
Updated. |
GuillaumeGomez
force-pushed
the
generics_span
branch
3 times, most recently
from
983a75a
to
31b3263
Compare
|
After discussion on IRC with @jseyfried and @KiChjang, I'll replace |
| fld: &mut T) | ||
| -> Generics { | ||
| Generics { | ||
| ty_params: fld.fold_ty_params(ty_params), | ||
| lifetimes: fld.fold_lifetime_defs(lifetimes), | ||
| where_clause: fld.fold_where_clause(where_clause), | ||
| span: span, |
There was a problem hiding this comment.
This should also be folded with fld.new_span().
GuillaumeGomez
force-pushed
the
generics_span
branch
from
31b3263
to
edfa1c1
Compare
|
Updated. |
GuillaumeGomez
force-pushed
the
generics_span
branch
3 times, most recently
from
690b974
to
c060850
Compare
|
☔ The latest upstream changes (presumably #35747) made this pull request unmergeable. Please resolve the merge conflicts. |
GuillaumeGomez
force-pushed
the
generics_span
branch
from
c060850
to
0a1aff9
Compare
| @@ -4273,7 +4274,9 @@ impl<'a> Parser<'a> { | |||
| where_clause: WhereClause { | |||
| id: ast::DUMMY_NODE_ID, | |||
| predicates: Vec::new(), | |||
| } | |||
| }, | |||
| // We lessen by BytePos(1) because self.span is already the element following ">". | |||
There was a problem hiding this comment.
nit: I'm not 100% sure I understand what this comment means. Are you saying the span includes the ">" and we don't want it?
There was a problem hiding this comment.
No, For example:
fn f<T>(T) {}In here, after the function call, the span is ")" instead of ">". So self.span is the following element.
There was a problem hiding this comment.
Could you use self.last_span.hi instead of self.span.hi - BytePos(1)?
|
@Manishearth - it looks ready for r=me when you're ready. I only had minor comments. |
|
Cool. These should all get batched up soon. |
GuillaumeGomez
force-pushed
the
generics_span
branch
from
0a1aff9
to
5948182
Compare
|
Updated to @jseyfried great suggestion! |
|
Switching to Manishearth so he can do the batching... r? @Manishearth |
Add Span field for Generics structs
Batch up libsyntax breaking changes Batch of the following syntax-[breaking-change] changes: - #35591: Add a field `span: Span` to `ast::Generics`. - #35618: Remove variant `Mod` of `ast::PathListItemKind` and refactor the remaining variant `ast::PathListKind::Ident` to a struct `ast::PathListKind_`. - #35480: Change uses of `Constness` in the AST to `Spanned<Constness>`. - c.f. `MethodSig`, `ItemKind` - #35728: Refactor `cx.pat_enum()` into `cx.pat_tuple_struct()` and `cx.pat_path()`. - #35850: Generalize the elements of lists in attributes from `MetaItem` to a new type `NestedMetaItem` that can represent a `MetaItem` or a literal. - #35917: Remove traits `AttrMetaMethods`, `AttributeMethods`, and `AttrNestedMetaItemMethods`. - Besides removing imports of these traits, this won't cause fallout. - Add a variant `Union` to `ItemKind` to future proof for `union` (c.f. #36016). - Remove inherent methods `attrs` and `fold_attrs` of `Annotatable`. - Use methods `attrs` and `map_attrs` of `HasAttrs` instead. r? @Manishearth
r? @jonathandturner