Uh oh!
There was an error while loading. Please reload this page.
53956 panic on include bytes of own file - #54517
Conversation
rust-highfive
commented
Sep 24, 2018
r? @eddyb (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
commented
Sep 24, 2018
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 |
matthew-russo
commented
Sep 24, 2018
Will do a full stage 2 build/test and debug |
730e62b to
5a6421dComparematthew-russo
commented
Sep 27, 2018
eddyb
commented
Sep 28, 2018
michaelwoerister
commented
Oct 1, 2018
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! |
michaelwoerister
commented
Oct 4, 2018
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? |
matthew-russo
commented
Oct 4, 2018
@michaelwoerister thanks for the feedback. I’ll make the updates and then tag you when it’s ready for review. |
michaelwoerister
commented
Oct 6, 2018
Thanks, @mcr431! |
5a6421d to
8ebe646Comparematthew-russo
commented
Oct 10, 2018
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 |
rust-highfive
commented
Oct 10, 2018
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 |
matthew-russo
commented
Oct 15, 2018
@michaelwoerister can i get some help from you or someone else on this? I made the changes so |
There was a problem hiding this comment.
You shouldn't need to clone the bytes vec.
There was a problem hiding this comment.
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.
Oh right, I didn't see that. Nevermind then.
There was a problem hiding this comment.
I would be good to make new just call new_from_pieces.
There was a problem hiding this comment.
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>.
michaelwoerister
commented
Oct 23, 2018
Hm, I don't know yet why this is failing. Maybe proc-macros rely on being able to "overwrite" files in the |
matthew-russo
commented
Oct 23, 2018
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 |
michaelwoerister
commented
Oct 23, 2018
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. |
matthew-russo
commented
Oct 23, 2018
@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 SourceFiles 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
4c5b8c7Comparematthew-russo
commented
Oct 23, 2018
@michaelwoerister noted. I made a couple of the changes you suggested. |
michaelwoerister
commented
Nov 30, 2018
Awesome, thank you! |
rust-highfive
commented
Dec 4, 2018
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 |
matthew-russo
commented
Dec 4, 2018
Hmmm. something smells off. everything is green locally. will debug and update |
ba1d347 to
223ea10Compare…s includes the source if it can convert bytes to string
…_map, fixes remaining test failures
223ea10 to
f0f8aa9Comparematthew-russo
commented
Dec 5, 2018
WOOHOOOO finally! @michaelwoerister can you check it out and make sure my code changes are acceptable? |
michaelwoerister
commented
Dec 5, 2018
Looks great! Thanks a lot for keeping at it for so long, @mcr431. @bors r+ |
bors
commented
Dec 5, 2018
📌 Commit f0f8aa9 has been approved by |
bors
commented
Dec 6, 2018
…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.
bors
commented
Dec 6, 2018
☀️ Test successful - status-appveyor, status-travis |
michaelwoerister
commented
Dec 6, 2018
🎉 |
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_byteswould overwrite files in the source_map with no source. This PR changesexpand_include_bytesto check source_map and use the already existing src, if any.