Add a compiler option for a 'pseudo-cwd' for rustc errors

[alexcrichton]

Cargo's behavior of what the cwd is when rustc is invoked changed in rust-lang/cargo#4788 for a number of reasons, but this has caused a number of regressions in tooling because the paths printed in error messages are now relative to a different location.

In talking with @nikomatsakis an idea that came up was that we could add an option to the compiler to say "print diagnostic paths relative to this location". Cargo could pass the current cwd of the cargo process itself when invoking rustc and the compiler would otherwise know to not encode this value into the output artifact at all.

cc @estebank, you may be interested in this as well!

[nikomatsakis]

cc @rust-lang/compiler

[estebank]

This should be useful for any tooling that deals with the compiler output, even beyond cargo.

[eddyb]

Should this be tied/related to the debuginfo path renaming options? (cc @michaelwoerister)

[alexcrichton]

In this context @eddyb I don't think so. That was actually the bugfix in Cargo because Cargo was incorrectly caching artifacts when it otherwise would have changed because the debuginfo would have been different. Instead Cargo now produces artifacts with deterministic debuginfo (regardless of the cwd of who invoked Cargo to build the crate), but for tooling it seems like it may be best to continue to produce diagnostics relative to the cwd

[nikomatsakis]

Question is: who has time to hack this up =)

[ehuss]

Would/should this option also affect the file! macro?

I parse the output from tests to help the user jump to any failed tests, so it needs to resolve those relative paths as well.

[alexcrichton]

@ehuss since that affects the actual output binary itself i'd imagine that this flag wouldn't affect that (as then Cargo would go back to recompiling whenever cwd changed!)

[oli-obk]

How about making this an env var, so tooling could use this without caring how to pass down the path through arbitrary layers of scripts an tools?

This way no part of the compiler has to know about it except for the diagnostic emitters

[nikomatsakis]

I agree with @alexcrichton that this should purely affect compiler output.

I don't know how I feel about using an environment variable. I guess it might be ok; in general, though, environment variables always feel sort of like "action at a distance" to me.

[nikomatsakis]

triage: P-high

If we're going to do something here, it should be soon!

What is the status in terms of other tools (e.g., IDEs) getting mitigations?

[ehuss]

I looked at this briefly. I'm uncertain if you have ideas about how to fix it. I was thinking maybe the paths could be rebased at the last moment inside the Emitters. However, I feel like that could be a very brittle solution, since it would be easy for paths to slip by, or future changes to miss adding the translation. I'm very unfamiliar with the compiler, so I don't know what gotchas might be hiding.

As for Sublime, I've already migrated it to using the metadata. It would not be difficult for me to switch back, though.

Finally, are we certain this is the right solution? It would add inconsistencies (symbols and file!/panic would be one way, diagnostics another). I can't think of a specific drawback (other than some IDEs will need to deal with both), but at least for Sublime I think I can handle it.

[michaelwoerister]

This would certainly interact with path remapping, which is already implemented. Path remapping affects debuginfo and error messages. Maybe it could be used to achieve the desired effect? Here's how to use it: https://doc.rust-lang.org/unstable-book/compiler-flags/remap-path-prefix.html

[michaelwoerister]

Before using path remapping from cargo though, we should finally stabilize it (stabilization has been approved for a while, I just never found the time to implement the necessary changes).

[pnkfelix]

In my experimenting so far, I was assuming this flag should not affect debuginfo. To my mind, debuginfo is a build artifact that needs to be stable. (I am talking about this now with @michaelwoerister in one-on-one...)

[pnkfelix]

@ehuss I don't know what your thoughts are on this; my plan for catching paths from slipping by was to remove the Display impl from syntax_pos::FileName. Its not fool-proof since people might still match on the enum FileName itself and extract the underlying path that way, but it seems to catch a lot of cases in practice from what I've seen.

[ehuss]

my plan for catching paths from slipping by was to remove the Display impl from syntax_pos::FileName

@pnkfelix That sounds like a good idea!

Have you decided how it will interact with the existing path remapping?

[matklad]

What is the status in terms of other tools (e.g., IDEs) getting mitigations?

I am of the opinion that this probably should just be fixed on the tools side.

If the tools have to learn about this env-var/option anyway, than perhaps they can just interpret the new paths correctly?

IntelliJ's nightly version already supports old and new flavors of paths, and today's release should bring this support to users of stable IntelliJ.

[matklad]

Fix for cargo.el: kwrooijen/cargo.el#55

[pnkfelix]

I am not clear what conclusion to draw from the dialogue on kwrooijen/cargo.el#55 ; it sounds like it is being suggested there that cargo should be modified in further ways to accommodate various IDEs. But at this point I'm still hoping that the suggestion provided in this issue (of adding a flag to rustc that indicates an extra prefix representing the path to follow to get from the driver's cwd over to the rustc cwd) is actually workable, even if I cannot get it working in time for us to save the change to the stable channel.

(Though I freely admit that perhaps if the problem leaks out to stable then doing the above will be just a waste of effort in the end? I suppose that depends on how easy we want to make it for IDE's to bring back the prior behavior, even if they have to wait a release cycle before they can do so...)

[matklad]

@pnkfelix to clarify, it seems to me that kwrooijen/cargo.el#55 does not need any new features from Cargo/Rustc or Emacs, it just needs someone experienced with elisp to put all the blocks into a workable solution :-)

There are actually two solutions possible:

• add --manifest-path path/to/Cargo.toml over here: https://github.com/matklad/cargo.el/blob/b74750b82ef88bcec13a5f20c8094ac44e948a40/cargo-process.el#L233

• mess up with compilation-error-regexp-alist-alist which looks like it does allow to plug-in custom logic for resolving paths.

I would probably try to push the first one over the finish line, but I sort-of hope that someone how actually uses cargo.el does this instead :)

[pnkfelix]

@matklad I guess my main problem is that I worry about the impact of asking every IDE to adjust their system, and potentially have to do so in a way where they have to remain backwards compatible with old versions of rust... I'd rather, if possible, give a really simple path to recovering the old behavior. (Which is why I've been trying to hack up an appropriate flag)

[nikomatsakis]

@pnkfelix one (small, perhaps) counterpoint: at least in emacs, because the strings in the binary have changed, RUST_BACKTRACE=1 output now gives the strings relative to the Cargo.toml. (So even if rustc rewrote its errors, there would still be a desire to adjust its behavior.)

I do have some sense thatthe "time" to do this change has slowly slipt away, though it still seems ultimately like a potentially handy feature for rustc to make available, just in general.

[pnkfelix]

I do have some sense thatthe "time" to do this change has slowly slipt away, though it still seems ultimately like a potentially handy feature for rustc to make available, just in general.

Yes, I too am thinking that if if this had been a hack I had been able to put together last Friday, then maybe we could have pushed it through in time to address this problem.

But its instead looking like we are going just have to say "mea culpa" to our clients, and (at best) try to fix the IDE's for which we offer support config files ?

[michaelwoerister]

Honestly, this seems a bit too hacky to me. If it can be solved on the tools side then I'd rather not have this flag.

[nikomatsakis]

triage: P-medium

We are not going to solve the regression. This is now a question of whether we want this feature or not. I'm of mixed minds.

For rustc itself, I think the root problem is that Cargo.toml is in the src directory instead of living alongside x.py.

[pnkfelix]

(Note that discussion of the bug that this was attempting to resolve is ongoing at rust-lang/cargo#4998 ; I plan to use that as a reference point for any fixes for x.py, cargo.el, etc)

[pnkfelix]

I'm also going to close this issue since I'm not planning on trying to adopt this fix for use in cargo.el nor in the x.py workflow. However if someone else reopens this ticket (i.e. because they think this feature would be of use for their own IDE), then at that point I'll polish off my PR for this feature.

In the meantime, I've posted the current state of that branch here: https://github.com/pnkfelix/rust/tree/cwd-prefix-for-diagnostics-issue-47339-chkpt