Improve error messages for wrong target paths #9116
Conversation
As it was stated in rust-lang#9014 it's not really useful the message that you get when using `bench/your_bench.rs` instead of the correct path `benches/your_bench.rs`. Therefore, as @ehuss suggested, the error message has been improved not only suggesting the correct path but also suggesting to specify `bench.path` in `Cargo.toml` for non-default path usage. And this has been exented to `test`, `example` and `bin`.
|
r? @ehuss (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
CPerezz
force-pushed
the
better_bench_err
branch
from
207f71e
to
5ba1412
Compare
| Err(format!( | ||
| "can't find `{name}` {target_kind}, specify {target_kind}.path", | ||
| "can't find `{name}` {target_kind} at `{target_folder_name}/{name}`, specify {target_kind}.path if you want to use a non-default path", |
There was a problem hiding this comment.
Could this perhaps use tthe inferred list at the top to generate a precise list of candidates attempted? Otherwise this misses out that either benches/foo.rs or benches/foo/main.rs could exist. (e.g. benches/foo could exist when this says it doesn't b/c you happened to be missing benches/foo/main.rs)
There was a problem hiding this comment.
Hey @alexcrichton Thanks for the catch!! I didn't thought about that!
There's only one problem. This slice is constructed here
And as you see, infer_from_directory you send the benches dir to the fn. And obviously, that resolves into an empty array because the bench is located under bench/ and not benches.
I don't really see how to tackle that. Since we need to "guess" which is the wrong path. And if we parse from package_root looking for similar names to benches, we might end up in undesired places.
Which is your suggestion?
If you want to clarify even more, We can go for a suggestion message below the err saying something like:
Please, make sure to name your target folder correctly. \
For {target_kind} the folder should be called `{target_folder_name}/..`
There was a problem hiding this comment.
Ah that's a good point. I think there will need to be some refactoring for this in any case though. The error message is already slightly incorrect in that if we found two candidates it says that something wasn't found when actually we found two things. Similarly I'm worried about the catch-all _ => target_kind here since that may produce incorrect messages trying to look in a folder like lib/foo for example.
There was a problem hiding this comment.
It shouldn't IIUC. The catch all is precisely what handles lib to be matched to lib itself. The others are the ones which can contain wrong folder names. But maybe I'm missing something.
We could parse the top level of the workspace looking for example, test, bins or bench folders. If that's the case, we try to extract inferred from there and we will be able to formulate a better error message. Otherways. I really don;t know what we can do. Since literally the path could be whatever.
Do you have any similar situations in other places of the crate? Do you have any suggestions on what to go for?
There was a problem hiding this comment.
There is a set of paths that Cargo probes for for targets which don't list a path. That set I think should be refactored to be able to be passed in here explicitly so this doesn't have to reconstruct or re-infer what the missing paths were. I think that refactoring would solve all of my concerns here.
There was a problem hiding this comment.
Thanks for the explanation Alex. Could you point where Cargo probes for for targets which don't list a path? I've been looking across the repo for something similar to a Layout structure where these targets without listed path are treated or stored and haven't seen it.
I checked from the creation of the Workspace till the execution of the benches and nothing seemed to me like was cheking this "missed" targets.
That would be really helpful for me so I can make another PR to refactor this and then, use that so that the error message gets improved.
I can also include the refactor here, whatever you prefer.
There was a problem hiding this comment.
I think the probe logic is in the functions in this module (inferred_bins, inferred_lib, infer_from_directory). I think the refactoring would be to change those to return a list of potential candidates (essentially, remove the checks for path.exists()), and then the code that creates the targets (toml_targets_and_inferred I think) will need to check for the first path in that candidate list that exists (something like inferred.iter().find(|p| p.exists())). Then, you can pass that list of candidates to target_path so that it can include them in the error message.
It may need to be careful since those paths are absolute. I'm not sure how that should be displayed, but maybe strip the current directory from the prefix?
ehuss
added
S-waiting-on-author
|
I'm gonna close this due to inactivity. Feel free to reopen or create a new PR when you've got time to work on this again. Thanks! |
Enhance error message for target auto-discovery resolves #9014 resolves #9117 Enhance for following scenarios: 1. Target without `path` specified and cannot be found. 2. Target without `path` specified and cannot be found, but a file exists at the commonly wrong path, e.g. `example/a.rs`, `bench/b.rs`. 3. Found multiple candidate files and cannot infer which to use. For the suggestion in [the thread in #9116], I can't see any feasible way to list potential candidates without addditional I/O checking file existences. This PR is the best effort I can think of at this time. Feel free to comment. Thanks! [the thread in #9116]: #9116 (comment)
As it was stated in #9014 it's not really useful the message
that you get when using
bench/your_bench.rsinstead of thecorrect path
benches/your_bench.rs.Therefore, as it was suggested, the error message has been improved
not only suggesting the correct path but also suggesting to
specify
bench.pathinCargo.tomlfor non-default path usage.And this has been exented to
test,exampleandbin.Closes #9014