-
Notifications
You must be signed in to change notification settings - Fork 13k
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
53956 panic on include bytes of own file #54517
53956 panic on include bytes of own file #54517
Conversation
r? @eddyb (rust_highfive has picked a reviewer for you, use r? to override) |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Will do a full stage 2 build/test and debug |
730e62b
to
5a6421d
Compare
Hm, ok, I think I understand the root cause now. I'll need to think a little more before I can decide if this is a good fix. Thanks for looking into this, @mcr431! |
OK, so the underlying problem is that the same file can get added to the source map twice, once via the regular parser and once via an include macro. If the macro includes the file after the parser has already added it, it will overwrite the entry in the source map's The changes in the PR fix this problem by passing the correct source to the duplicate entry. However, I think the cleaner fix would be to safeguard You'll probably need to add another constructor to Would you be up to adapting your PR accordingly, @mcr431? |
@michaelwoerister thanks for the feedback. I’ll make the updates and then tag you when it’s ready for review. |
Thanks, @mcr431! |
5a6421d
to
8ebe646
Compare
I am not expecting this to build. Stage 1 test passes but Stage 2 has failures with proc macro. pushed for visibility and so i can work across machines |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@michaelwoerister can i get some help from you or someone else on this? I made the changes so |
// Add this input file to the code map to make it available as | ||
// dependency information, but don't enter it's contents | ||
cx.source_map().new_source_file(file.into(), String::new()); | ||
let src = match String::from_utf8(bytes.clone()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You shouldn't need to clone the bytes
vec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is cloned because bytes
is also used to make both the src string and the LitKind::ByteStr ast node at the end of the method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right, I didn't see that. Nevermind then.
src/libsyntax/source_map.rs
Outdated
@@ -118,6 +118,18 @@ impl StableFilemapId { | |||
|
|||
StableFilemapId(hasher.finish()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be good to make new
just call new_from_pieces
.
src/libsyntax/source_map.rs
Outdated
|
||
pub fn new_from_pieces(name: &FileName, | ||
name_was_remapped: bool, | ||
unmapped_path: Option<FileName>) -> StableFilemapId { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unmapped_path
should be passed as a reference (either &Option<FileName>
or Option<&FileName>
). Then you don't have to clone it. You can use Option::as_ref()
to convert from Option<T>
to Option<&T>
.
Hm, I don't know yet why this is failing. Maybe proc-macros rely on being able to "overwrite" files in the |
That was my initial assumption. I naively tried to have new_source_file just update the src property if it already existed but it wasn't working because there was another mutable reference to it elsewhere. I'll make the adjustments you commented on |
So, yes, it looks like proc-macros all have the same "filename" and rely on the source map for parsing. I'm surprised that this hasn't caused any problems with incremental compilation yet. |
@michaelwoerister Good to know. Where did you find that just so I can get a better idea? And should I change it so each proc macro gets its own file or did you have a different idea? |
Lines 86 to 106 in d74b402
Since the ProcMacroSourceCode variant has no field to differentiate between different invocations, all "file names" of SourceFile s generated for proc-macro invocations will be the same.
We could add a field that somehow identifies the invocation but that has implications for incremental compilation. I'm not sure yet how to best solve this. |
8ebe646
to
4c5b8c7
Compare
@michaelwoerister noted. I made a couple of the changes you suggested. |
Awesome, thank you! |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Hmmm. something smells off. everything is green locally. will debug and update |
ba1d347
to
223ea10
Compare
…s includes the source if it can convert bytes to string
…_map, fixes remaining test failures
223ea10
to
f0f8aa9
Compare
WOOHOOOO finally! @michaelwoerister can you check it out and make sure my code changes are acceptable? |
Looks great! Thanks a lot for keeping at it for so long, @mcr431. @bors r+ |
📌 Commit f0f8aa9 has been approved by |
…e, r=michaelwoerister 53956 panic on include bytes of own file fix #53956 When using `include_bytes!` on a source file in the project, compiler would panic on subsequent compilations because `expand_include_bytes` would overwrite files in the source_map with no source. This PR changes `expand_include_bytes` to check source_map and use the already existing src, if any.
☀️ Test successful - status-appveyor, status-travis |
🎉 |
I believe this variant name was used incorrectly. The timeline is roughly: * `FileName::cfg_spec_source_code` was added in rust-lang#54517. However, it used `FileName::Quote` instead of `FileName::CfgSpec` which I believe was a mistake. * Quote stuff was removed in rust-lang#51285, but did not remove `FileName::Quote`. * `FileName::CfgSpec` was removed in rust-lang#116474 because it was unused. This restores it so that the `--cfg` variant uses a name that makes more sense with how it is used, and restores what I think is the original intent.
Rename FileName::QuoteExpansion to CfgSpec I believe this variant name was used incorrectly. The timeline is roughly: * `FileName::cfg_spec_source_code` was added in rust-lang#54517. However, it used `FileName::Quote` instead of `FileName::CfgSpec` which I believe was a mistake. * Quote stuff was removed in rust-lang#51285, but did not remove `FileName::Quote`. * `FileName::CfgSpec` was removed in rust-lang#116474 because it was unused. This restores it so that the `--cfg` variant uses a name that makes more sense with how it is used, and restores what I think is the original intent.
Rollup merge of rust-lang#135747 - ehuss:filename-quote, r=SparrowLii Rename FileName::QuoteExpansion to CfgSpec I believe this variant name was used incorrectly. The timeline is roughly: * `FileName::cfg_spec_source_code` was added in rust-lang#54517. However, it used `FileName::Quote` instead of `FileName::CfgSpec` which I believe was a mistake. * Quote stuff was removed in rust-lang#51285, but did not remove `FileName::Quote`. * `FileName::CfgSpec` was removed in rust-lang#116474 because it was unused. This restores it so that the `--cfg` variant uses a name that makes more sense with how it is used, and restores what I think is the original intent.
fix #53956
When using
include_bytes!
on a source file in the project, compiler would panic on subsequent compilations becauseexpand_include_bytes
would overwrite files in the source_map with no source. This PR changesexpand_include_bytes
to check source_map and use the already existing src, if any.