Expose manifest error chain in CargoErrors

[alexheretic]

Adds new ManifestErrors to the CargoError causal chain. These errors pass on their display, but provide more detail on which manifests are at fault when failing to build a Workspace. This is useful for lib users, in particular rls, allowing lookup of a particular manifest file that caused the error. See #6144.

For example a workspace foo where a member bar has an invalid toml manifest would have the error chain:

• failed to parse manifest at /home/alex/project/foo/bar/Cargo.toml
  ManifestError: /home/alex/project/foo/Cargo.toml
• failed to parse manifest at /home/alex/project/foo/bar/Cargo.toml
  ManifestError: /home/alex/project/foo/bar/Cargo.toml
• failed to parse manifest at /home/alex/project/foo/bar/Cargo.toml
• could not parse input as TOML
• expected a value, found an equals at line 8

This will allow rls to point to a particular workspace member's manifest file when that manifest fails to deserialize, has invalid paths, etc.

This change should not affect binary use.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matklad (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[alexcrichton]

Thanks for the PR! I wonder though if the filtering of "don't wrap I/O errors" can happen in a different location? Ideally we'd deterministically wrap an error in a manifest error after a certain point instead of filtering after-the-fact (which seems like it could be lossy)

[alexheretic]

Yes i think that probably can be removed, it'll mean finding the correct manifest to plonk the error on will be a little harder.

But it also makes it simpler.

[alexcrichton]

Looks like some tests are failing, but is this also the best place perhaps? I would have expected error wrapping like this to happen somewhere within src/cargo/util/toml/*.rs as that's where all our various toml-parsing utilities are

[alexheretic]

Simply wrapping in read_manifest would only wrap the root cause I think, rather than produce a chain of manifest errors. The chain is important so I can pick the nearest manifest inside the project for highlighting.

However, the initial manifest error could come from there. I'll see if that works.

[alexcrichton]

Thanks! I'm not really sure what the displayable part is doing now though, was that necessary to keep the tests passing? (it's also ok to just update the output of the tests)

[alexheretic]

Well these errors provide a chain of info to lib users, they don't add any "displayable" info for the message / bin users. Because of this I thought it made sense to filter them out of the text output, plus having them in caused a bunch of tests to fail as some error messages became duplicated.

I added the trait to make the meaning clearer, rather than filtering ManifestErrors directly. This'll make it easier to add another such error chain to other areas, which I'd like to do with the compile errors.

[alexcrichton]

I think we have at least one or two instances of "marker errors" in Cargo, and I think those strategies could be used to avoid duplication of error messages? I think the internal constructor is an example of one which flags an error as internal but doesn't affect the backtrace of displayed errors that show up

[alexheretic]

The difference here though is that I'm intending to change the backtrace. The backtrace is added to in order to trace the manifests involved in an error.

[alexcrichton]

Yes it should be possible to have type-level markers in the backtrace which don't affect how it's printed (without the addition of a new method). I believe that's how internal errors work in Cargo today (although they're specifically used to change printing, but only because it's special cased)

[alexheretic]

The wrapped error indeed doesn't affect the display. The filtering is used when we're printing each cause in the chain. ManifestErrors will have the same display as their cause, so if you just print the chain you get a bunch of duplication.

• failed to parse manifest at /home/alex/project/foo/bar/Cargo.toml
• could not parse input as TOML
• expected a value, found an equals at line 8

becomes

• failed to parse manifest at /home/alex/project/foo/bar/Cargo.toml
• failed to parse manifest at /home/alex/project/foo/bar/Cargo.toml
• failed to parse manifest at /home/alex/project/foo/bar/Cargo.toml
• could not parse input as TOML
• expected a value, found an equals at line 8

When printing out the displays this isn't useful, hence the filtering. Yet I do want this "duplication", because each extra link has extra info. In particular which manifest inside a project is "nearest" to the error.

I guess I'm doing a bad job at explaining this. I feel like I'm dancing around the same points and probably missing what you're after.

[alexcrichton]

@alexheretic er sorry well my point is that displayable probably does not need to be added to this PR. That's some extra infrastructure being added in this PR which is going to be difficult to maintain going forward. I think you can instead remove DisplayableError and fix the problem with duplicate outputs of errors in the error chain printing elsewhere.

I would recommend looking at Internal, which is a type-level marker for an error which doesn't affect the output display chain.

[alexheretic]

As I'm currently using the cause-chain to get the manifests involved I don't think Internal will work for this, it doesn't affect the display chain because it takes the place of it's cause and doesn't allow adding potentially lots of links.

Perhaps I could provide a different chain of ManifestErrors outside of the cause-chain. I'll give it a try anyway.

[alexheretic]

Ok maybe this is what you're after, now lower impact outside of the intended usage as a new method ManifestError::manifest_causes returns the ManifestError chain I need, but general cause chain acts the same way as Internal as you suggested.

[alexcrichton]

@bors: r+

Ah yeah that looks good to me, thanks so much!

[bors]

📌 Commit 25b7ad2 has been approved by alexcrichton

[bors]

⌛ Testing commit 25b7ad2 with merge 04c689ebf17be091e7a173034422e4fee5a0a208...

[bors]

💔 Test failed - status-appveyor

[alexcrichton]

@bors: retry

[bors]

⌛ Testing commit 25b7ad2 with merge 6d4be78...

[alexheretic]

@alexcrichton thanks for patiently reviewing this!

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 6d4be78 to master...