Skip to content
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

Update the disambiguation handling in RFC 1946 (intra-rustdoc-links) to match impl concerns #2285

Merged

Conversation

Manishearth
Copy link
Member

In rust-lang/rust#47046, we realized that the proposed disambiguation syntax is not valid CommonMark, and changed it to [struct@Foo], etc.

Additionally, we feel that you should be allowed to use un-disambiguated paths to refer to values as long as there isn't a clash.

cc @QuietMisdreavus @killercup

@Manishearth Manishearth added the T-rustdoc Relevant to T-dev-tools (specifically rustdoc), which will review and decide on the RFC. label Jan 10, 2018
@Manishearth
Copy link
Member Author

AFAICT this doesn't need to go through the full RFC process, just a quick discussion and perhaps fcp.

@@ -317,6 +320,7 @@ with the wrong prefix that is in the same namespace.
E.g., given an `struct Foo`, it may be possible to link to it using `[enum Foo]`,
or, given a `mod bar`, it may be possible to link to that using `[struct bar]`.

In case a type is in
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dangling line?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my bad, was initially going to expand there and then decided to take a different path. removed.

@est31
Copy link
Member

est31 commented Jan 10, 2018

@Manishearth awesome PR, fewest RFCs get corrected if it is decided that some other path should be taken.

Copy link
Member

@killercup killercup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! <3

- Links to `struct`s can be prefixed with `struct `,
e.g., `See [struct Foo]`.
e.g., `See [struct@Foo]`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use the @ in the link target but allow the space when using implied ref links?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but I prefer uniformity here. Wonder what others think.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I like your idea better. Hm.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I actually wrote the implementation with that in mind when i switched it to use @ >_>)

@kennytm
Copy link
Member

kennytm commented Jan 10, 2018

Apparently, links with spaces in them don't work in commonmark at all.

Interestingly escaping the space like struct\ Foo does work (and the URL is encoded as %5C%20 😒)

In this crate we would like to link to:

* [`ThisType`](struct\ ::ThisType)
* [`ThisEnum`](enum\ ::ThisEnum)
* [`ThisTrait`](trait\ ::ThisTrait)
* [`ThisAlias`](type\ ::ThisAlias)
* [`ThisUnion`](union\ ::ThisUnion)
* [`this_function`](::this_function())
* [`THIS_CONST`](const\ ::THIS_CONST)
* [`THIS_STATIC`](static\ ::THIS_STATIC)
<p>In this crate we would like to link to:</p>
<ul>
<li><a href="struct%5C%20::ThisType"><code>ThisType</code></a></li>
<li><a href="enum%5C%20::ThisEnum"><code>ThisEnum</code></a></li>
<li><a href="trait%5C%20::ThisTrait"><code>ThisTrait</code></a></li>
<li><a href="type%5C%20::ThisAlias"><code>ThisAlias</code></a></li>
<li><a href="union%5C%20::ThisUnion"><code>ThisUnion</code></a></li>
<li><a href="::this_function()"><code>this_function</code></a></li>
<li><a href="const%5C%20::THIS_CONST"><code>THIS_CONST</code></a></li>
<li><a href="static%5C%20::THIS_STATIC"><code>THIS_STATIC</code></a></li>
</ul>

the case of there being a value in both the type and value namespace.
Non-disambiguated paths cannot be used to link to macros.
- Links to types can be disambiguated by prefixing them with the concrete
item type:
- Links to `struct`s can be prefixed with `struct `,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since all these "can be prefixed with" notes include a space in their code span, i would like to see the @ in there as well. So this one would be:

Links to structs can be prefixed with struct@,

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@liigo
Copy link
Contributor

liigo commented Jan 18, 2018

[struct.Foo] is better since the dot (.) don't requires you pressing the SHIFT key.

@Manishearth
Copy link
Member Author

The problem is that struct.foo is more like a URL. We eventually want to detect and warn on broken intra links, and to do that you need to be sure that something was a broken intra link and not a URL. For this there should be a clear way of disambiguating, "optionally-backtick-surrounded identifier with possible at symbol or parentheses" doesn't clash with what most URLs will look like.

@Manishearth
Copy link
Member Author

r? @rust-lang/dev-tools

@nrc
Copy link
Member

nrc commented Jan 19, 2018

@Fcpbot fcp merge

I'm not quite happy to merge, but I think we should start FCP now and discuss as we go since this is just an amendment to an existing RFC (discussed at dev-tools meeting today).

@nrc
Copy link
Member

nrc commented Jan 19, 2018

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Jan 19, 2018

Team member @nrc has proposed to merge this. The next step is review by the rest of the tagged teams:

Concerns:

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added the proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. label Jan 19, 2018
applies to modules and tuple structs which exist in both namespaces.
Rustdoc will throw an error if you use a non-disambiguated path in
the case of there being a value in both the type and value namespace.
Non-disambiguated paths cannot be used to link to macros.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what the current impl is, this amendment matches the impl. That can be changed.


For disambiguation markers using an `@`, in implied shortcut links
you can use a space instead of the `@`. In other words, `[struct Foo]`
is fine (and preferred).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should note that you don't need to use the data type names - using type@ and value@ should be OK for all types and values, respectively.

- In links to macros,
the link label must end with a `!`,
the link label _must_ end with a `!`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't looked this up, but I'm pretty sure I argued strongly against this in the original RFC and I'm surprised to see it here. The ! is the syntax for calling a macro, not naming it (analogous to () to call a function), so I think rustdoc should not include the ! as part of the name (this is more important with macros 2.0 where you import the macro in a use statement without using !). Also, this doesn't make sense for attribute-like macros which are not even used with a !.

So, I would like to see macros named using just the name, and if disambiguation is necessary, to use macro@. I don't mind if you want to allow ! as a shorthand, but I would prefer not to.

- Links to type aliases can be prefixed with `type@`,
e.g., `See [type@foo]`.
- Links to modules can be prefixed with `mod@`,
e.g., `See [mod@foo]`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess unions and traits should be in this list too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

e.g., `See [type foo]`.
- Links to modules can be prefixed with `mod `,
e.g., `See [mod foo]`.
- In unambiguous cases paths can be written as described earlier,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iirc we discussed this in the original RFC and decided against it? I can't remember why though. One possible reason is forwards-compatability - adding a name to a namespace would break any docs that don't use a disambiguator.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can convert the error into a warning to make it a non-breaking change.

However, you already have this forwardcompat hazard with regular links. [foo](bar) is still a valid link; just won't point anywhere; usually.

You also can already break docs within this proposal by changing your imports in the file.

We provide disambiguation markers for folks who wish to catch this, and I think not mandating the disambiguation markers will make it easy to conveniently write docs. Clashing namespaces is not a common thing to do (aside from the implicit clashes introduced by tuple structs -- we handle this already), and when you do clash them usually it's to create a constructor for the same type so nbd.

@nrc
Copy link
Member

nrc commented Jan 19, 2018

@rfcbot concern review comments

@sophiajt
Copy link
Contributor

@nrc - I'm not on dev tools now, so I'm guessing my name should be removed?

@Manishearth
Copy link
Member Author

Manishearth commented Jan 19, 2018

The current impl e.g. allows you to use struct@ to refer to an enum, I'm not sure if we should spec this as always working or leave it implementation defined. I left a note making it implementation defined.

9eccc58

@Manishearth
Copy link
Member Author

I've made a change (f231d75) allowing ambiguous references to macros. I mostly didn't do this because it was tricky to impl and I wanted something working, but this can be changed.

@petrochenkov petrochenkov added T-dev-tools Relevant to the development tools subteam, which will review and decide on the RFC. and removed T-dev-tools labels Jan 30, 2018
@nrc
Copy link
Member

nrc commented Feb 1, 2018

We discussed at the dev-tools meeting today, we clarified that this should be regarded as a 'bug fix' RFC and that there will be a chance to re-visit and discuss the design during a stabilisation period. Therefore we should tick off and land this quickly.

@rfcbot resolved review comments

@Manishearth thanks for the updates!

@Manishearth
Copy link
Member Author

Oh, just a note, I also introduced method@ and variant@ in a fixup pr. I'll add that to the RFC before merging.

@Manishearth
Copy link
Member Author

Updated.

@Manishearth
Copy link
Member Author

(Updates on this? Stalled on review)

@fitzgen
Copy link
Member

fitzgen commented Feb 26, 2018

@rfcbot reviewed

(Sorry for delay!)

@killercup
Copy link
Member

I'm (still) in favor of this! :)

@rfcbot reviewed

@japaric
Copy link
Member

japaric commented Feb 26, 2018

@rfcbot reviewed

@rfcbot rfcbot added final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. and removed proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. labels Feb 26, 2018
@rfcbot
Copy link
Collaborator

rfcbot commented Feb 26, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot
Copy link
Collaborator

rfcbot commented Mar 8, 2018

The final comment period is now complete.

@Manishearth Manishearth merged commit fd70ea3 into rust-lang:master Mar 8, 2018
@Manishearth Manishearth deleted the intra-rustdoc-links-update branch March 8, 2018 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-dev-tools Relevant to the development tools subteam, which will review and decide on the RFC. T-rustdoc Relevant to T-dev-tools (specifically rustdoc), which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet