-
-
Notifications
You must be signed in to change notification settings - Fork 14.4k
rustdoc: Parse intra-doc links using rustc_parse
#151049
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
base: main
Are you sure you want to change the base?
Conversation
|
Blocked on #150586. |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
rustdoc: Parse intra-doc links using `rustc_parse`
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (aad88b3): comparison URL. Overall result: ❌ regressions - BENCHMARK(S) FAILEDBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @bors rollup=never ❗ ❗ ❗ ❗ ❗
❗ ❗ ❗ ❗ ❗ Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -2.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 6.5%, secondary 2.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 475.39s -> 474.205s (-0.25%) |
|
Ok, there are a couple of issues here:
The first could potentially be solved by not using Another complication, once we work on support for fully-qualified paths, is that Regarding backwards compatibility with keyword uses (e.g. Also, regarding backcompat with our non-standard primitive paths like Footnotes
|
|
cc @petrochenkov regarding the potential (minor) changes to the resolver that I mentioned above. |
I disagree here. Writing docs should be as easy as possible, and there is possible ambiguity here. I think the right way to handle this is to allow
I'm not too sure about this. I like |
|
If you make compiler changes related to doc link resolution, please assign those PRs to me |
I assume you meant there is no possible ambiguity. I initially thought that as well, and was thinking of adding a rustc_parse flag to allow keywords, but it's actually not true that there's no ambiguity. For example, fully-qualified paths use the Also, using keywords as item names is quite rare because it's a pain for people to write |
Yes, forgot the And good catch for the |
I don't really like the idea of special-casing Here's a possible approach:
Thoughts? |
|
Ideally, we'd run |
This will make it significantly easier to add support for fully-qualified paths (e.g. `<Foo as Trait>::bar` and `<dyn Trait>::bar`) without duplicating logic from rustc. All paths that need type-dependent resolution -- i.e. all that are not handled by `rustc_resolve` -- are now parsed by `rustc_parse`, except for associated items on some primitives. We support non-standard Rust syntax like `!::clone` in rustdoc, so those cannot be parsed by rustc. My hope is that once fully-qualified paths are implemented, we can deprecate (but still support) this unusual syntax. As part of this change, I also fixed a pre-existing bug where links to fields of re-exported variants were not resolved. This was because we assumed that these links always had at least three path segments; this assumption is broken by re-exports.
48794cf to
3003f3f
Compare
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
rustdoc: Parse intra-doc links using `rustc_parse`
|
The job Click to see the possible cause of the failure (guessed by this bot) |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (7018581): comparison URL. Overall result: ❌ regressions - BENCHMARK(S) FAILEDBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @bors rollup=never ❗ ❗ ❗ ❗ ❗
❗ ❗ ❗ ❗ ❗ Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -1.7%, secondary 5.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 470.684s -> 472.365s (0.36%) |
|
Much better after applying Vadim's suggestion of reusing the |
Fixes #151030.
This will make it significantly easier to add support for
fully-qualified paths (e.g.
<Foo as Trait>::barand<dyn Trait>::bar)without duplicating logic from rustc. All paths that need
type-dependent resolution -- i.e. all that are not handled by
rustc_resolve-- are now parsed byrustc_parse, except forassociated items on some primitives. We support non-standard Rust syntax
like
!::clonein rustdoc, so those cannot be parsed by rustc. My hopeis that once fully-qualified paths are implemented, we can deprecate
(but still support) this unusual syntax.
As part of this change, I also fixed a pre-existing bug where links to
fields of re-exported variants were not resolved. This was because we
assumed that these links always had at least three path segments; this
assumption is broken by re-exports.
r? @GuillaumeGomez