Skip to content

macros: improve Span's expansion information#40597

Merged
bors merged 5 commits into
rust-lang:masterfrom
jseyfried:improve_span_expn_info
Mar 30, 2017
Merged

macros: improve Span's expansion information#40597
bors merged 5 commits into
rust-lang:masterfrom
jseyfried:improve_span_expn_info

Conversation

@jseyfried

@jseyfried jseyfried commented Mar 17, 2017

Copy link
Copy Markdown
Contributor

This PR improves Span's expansion information. More specifically:

r? @nrc

@jseyfried

Copy link
Copy Markdown
Contributor Author

cc @eddyb
cc #38356 #39412

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

c.f. #40649; technically a [breaking-change]

@bors

bors commented Mar 19, 2017

Copy link
Copy Markdown
Collaborator

☔ The latest upstream changes (presumably #40346) made this pull request unmergeable. Please resolve the merge conflicts.

@jseyfried jseyfried force-pushed the improve_span_expn_info branch 2 times, most recently from e7e0177 to 72a3d9e Compare March 20, 2017 04:29
@bors

bors commented Mar 21, 2017

Copy link
Copy Markdown
Collaborator

☔ The latest upstream changes (presumably #40693) made this pull request unmergeable. Please resolve the merge conflicts.

@jseyfried jseyfried force-pushed the improve_span_expn_info branch from 72a3d9e to 061faa7 Compare March 21, 2017 21:50
@bors

bors commented Mar 22, 2017

Copy link
Copy Markdown
Collaborator

☔ The latest upstream changes (presumably #40043) made this pull request unmergeable. Please resolve the merge conflicts.

@jseyfried jseyfried force-pushed the improve_span_expn_info branch from 061faa7 to a30cfc0 Compare March 22, 2017 05:06
@bors

bors commented Mar 23, 2017

Copy link
Copy Markdown
Collaborator

☔ The latest upstream changes (presumably #40752) made this pull request unmergeable. Please resolve the merge conflicts.

@nrc nrc left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

r=me, assuming losing the command line span thing is fine.

This seems a good step in the right direction. I worry a bit that to is still too easy to create malformed spans, but it is def. better than mk_sp.

Comment thread src/libsyntax_pos/lib.rs Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What do we do now for code from the command line?

@jseyfried jseyfried Mar 24, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Same as before -- DUMMY_SP. The only place COMMAND_LINE_SP was used was here, which only happens when naming a legacy plugin crate through a debugging command line option.

@jseyfried jseyfried force-pushed the improve_span_expn_info branch 2 times, most recently from 81adb79 to 8c49446 Compare March 25, 2017 01:51
@jseyfried

Copy link
Copy Markdown
Contributor Author

@bors r=nrc

@bors

bors commented Mar 25, 2017

Copy link
Copy Markdown
Collaborator

📌 Commit 8c49446 has been approved by nrc

@jseyfried jseyfried force-pushed the improve_span_expn_info branch 5 times, most recently from 3618a0b to 8ba347e Compare March 30, 2017 01:17
@jseyfried

Copy link
Copy Markdown
Contributor Author

@bors r=nrc

@bors

bors commented Mar 30, 2017

Copy link
Copy Markdown
Collaborator

📌 Commit 8ba347e has been approved by nrc

@bors

bors commented Mar 30, 2017

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 8ba347e with merge 2731d4d...

@bors

bors commented Mar 30, 2017

Copy link
Copy Markdown
Collaborator

💔 Test failed - status-appveyor

@jseyfried jseyfried force-pushed the improve_span_expn_info branch from 8ba347e to cfad0fe Compare March 30, 2017 05:41
@jseyfried jseyfried force-pushed the improve_span_expn_info branch from cfad0fe to 8fde04b Compare March 30, 2017 06:48
@bors

bors commented Mar 30, 2017

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 8fde04b with merge fe15119...

bors added a commit that referenced this pull request Mar 30, 2017
macros: improve `Span`'s expansion information

This PR improves `Span`'s expansion information. More specifically:
 - It refactors AST node span construction to preserve expansion information.
   - Today, we only use the underlying tokens' `BytePos`s, throwing away the `ExpnId`s.
   - This improves the accuracy of AST nodes' expansion information, fixing #30506.
 - It refactors `span.expn_id: ExpnId` to `span.ctxt: SyntaxContext` and removes `ExpnId`.
   - This gives all tokens as much hygiene information as `Ident`s.
   - This is groundwork for procedural macros 2.0 `TokenStream` API.
   - This is also groundwork for declarative macros 2.0, which will need this hygiene information for some non-`Ident` tokens.
 - It simplifies processing of spans' expansion information throughout the compiler.
 - It fixes #40649.
 - It fixes #39450 and fixes part of #23480.

r? @nrc
@jseyfried

Copy link
Copy Markdown
Contributor Author

@bors r=nrc

@bors

bors commented Mar 30, 2017

Copy link
Copy Markdown
Collaborator

📌 Commit 8fde04b has been approved by nrc

@bors

bors commented Mar 30, 2017

Copy link
Copy Markdown
Collaborator

☀️ Test successful - status-appveyor, status-travis
Approved by: jseyfried
Pushing fe15119 to master...

@SergioBenitez

Copy link
Copy Markdown
Contributor

I'm seeing the following, which may be related to this:

warning: {warning message from a syntax extension}
--> /a/nice/big/file/path/here.rs:10:21
 |
 | and so on...
 |
 = note: this error originates in a macro outside of the current crate

The issue is that the "note:` refers to the warning as an "error". It should say: "this warning originates from a macro outside of the current crate".

@jseyfried

Copy link
Copy Markdown
Contributor Author

@sergio This should be fixed by #40939 (as discussed on IRC).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

macros: The line number that panic! reports can be wrong in macro invocations Spans for Paths can be incorrect

7 participants