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.

Sign up for GitHub

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

Jump to bottom

Use proper spans in ext::tt::transcribe #2887

Closed
catamorphism opened this issue Jul 13, 2012 · 17 comments · Fixed by #78603
Closed

Use proper spans in ext::tt::transcribe #2887

catamorphism opened this issue Jul 13, 2012 · 17 comments · Fixed by #78603
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-syntaxext Area: Syntax extensions C-cleanup Category: PRs that clean code up or issues documenting cleanup. P-low Low priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@catamorphism
Copy link
Contributor

Several FIXMEs (formerly TODOs) there that mention using the correct spans for different errors.
@pnkfelix
Copy link
Member

Not critical for 0.6; de-milestoning

@metajack
Copy link
Contributor

metajack commented May 9, 2013

FIXMEs are still there. I'm not sure if this should be nominated for a maturity milestone, but if it's something that will trip people up, production ready is probably appropriate.

@pnkfelix
Copy link
Member

pnkfelix commented Jul 1, 2013

Nominating for Milestone 5: Production Ready.

@graydon
Copy link
Contributor

graydon commented Jul 11, 2013

accepted for production-ready milestone

@pnkfelix
Copy link
Member

visiting for triage, email from 2013 sep 16

@pnkfelix
Copy link
Member

Accepting for P-low

@steveklabnik steveklabnik added the A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) label Jan 21, 2015
@steveklabnik
Copy link
Member

Triage: no change

@Manishearth
Copy link
Member

@nrc Is this still an issue?

@nrc
Copy link
Member

nrc commented Nov 15, 2015

yes

@fhahn fhahn mentioned this issue Jan 21, 2016
Merged
bors added a commit that referenced this issue Jan 27, 2016
@bors
Auto merge of #31089 - fhahn:macro-ice, r=pnkfelix
8256c47 
This is a  work in progress PR that potentially should fix #29084, #28308, #25385, #28288, #31011. I think this may also adresse parts of  #2887.

The problem in this issues seems to be that when transcribing macro arguments, we just clone the argument Nonterminal, which still has to original spans. This leads to the unprintable spans. One solution would be to update the spans of the inserted argument to match the argument in the macro definition. So for [this testcase](https://github.com/rust-lang/rust/compare/master...fhahn:macro-ice?expand=1#diff-f7def7420c51621640707b6337726876R2) the error message would be displayed in the macro definition:

    src/test/compile-fail/issue-31011.rs:4:12: 4:22 error: attempted access of field `trace` on type `&T`, but no field with that name was found
    src/test/compile-fail/issue-31011.rs:4         if $ctx.trace {

Currently I've added a very simple `update_span` function, which updates the span of the outer-most expression of a `NtExpr`, but this `update_span` function should be updated to handle all Nonterminals. But I'm pretty new to the macro system and would like to check if this approach makes sense, before doing that.
@Mark-Simulacrum
Copy link
Member

@nrc Did #31089 address this? The description of that PR mentions that it partially addressed this, but I don't know to what extent.

@nrc
Copy link
Member

nrc commented Sep 25, 2016

cc @pnkfelix, but I don't think this issue is entirely fixed, we still use some bad spans, that PR just fixed one case of them.

@steveklabnik steveklabnik added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 9, 2017
@Mark-Simulacrum
Copy link
Member

@jseyfried You've done a lot of work on this file recently, perhaps you could comment if there's still more to be done with the spans? src/libsyntax/ext/tt/transcribe.rs is the file for reference.

@jseyfried
Copy link
Contributor

I'm not familiar with the details off the top of my head, but IIRC there are comments about spans that could use improvements in the file.

@Mark-Simulacrum
Copy link
Member

That memory was correct; there are many FIXME comments in the file.

@estebank estebank mentioned this issue Jul 20, 2017
Merged
bors added a commit that referenced this issue Jul 22, 2017
@bors
Auto merge of #43352 - estebank:macro-span-replacement, r=petrochenkov
35f6499 
Use the macro structure spans instead of the invocation

Fix #42104, CC #2887.
@steveklabnik
Copy link
Member

There are now four FIXMEs in that file. No idea if that's few enough for this ticket or not.

@mark-i-m mark-i-m mentioned this issue May 7, 2019
Merged
@mark-i-m
Copy link
Member

mark-i-m commented May 7, 2019

If #60620 is correct and gets merged, we would be down to just one FIXME and it is about hygiene.

Specifically, when transcribing a MBE, when we replace a meta-variable with a matched nonterminal, currently:

  • If the meta-variable was a :tt, we don't do anything to the syntax context of token trees ($x:tt).
  • If the meta-variable is any other type of nonterminal, we apply the mark of the current expansion to the span of the nonterminal.

Why the distinction?

cc @petrochenkov

Centril added a commit to Centril/rust that referenced this issue May 10, 2019
@Centril
Rollup merge of rust-lang#60620 - mark-i-m:transcribe-2, r=petrochenkov
20dff29 
Fix a couple of FIXMEs in ext::tt::transcribe

_Blocked on #60618_

A crater run would be nice to make sure my understanding is correct. A quick google search seems to indicate these are extremely rare errors if they are possible (which I don't believe they are).

r? @petrochenkov

cc rust-lang#2887 (there is only one FIXME left and it is hygiene-related)
@petrochenkov petrochenkov mentioned this issue Oct 31, 2020
Merged
@petrochenkov
Copy link
Contributor

Why the distinction?

#78603 has an answer for this question.

@bors bors closed this as completed in 540d474 Nov 1, 2020
RalfJung pushed a commit to RalfJung/rust that referenced this issue Jun 29, 2023
@bors
Auto merge of rust-lang#2887 - Vanille-N:tb-mut-transmute, r=RalfJung
9667886 
TB: more fail tests (mostly shared with SB)

Although it was not in the tests, `mem::transmute` works for `UnsafeCell -> &` as well.

Draft: will also introduce more test cases for cases that fail.
Draft: depends on the new error messages from rust-lang#2888
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-syntaxext Area: Syntax extensions C-cleanup Category: PRs that clean code up or issues documenting cleanup. P-low Low priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

expand: Tweak a comment in implementation of macro_rules petrochenkov/rust