Use different numbers of `#`s when expanding documentation comments

[barosl]

Any documentation comments that contain raw-string-looking sequences may pretty-print invalid code when expanding them, as the current logic always uses the r"literal" form, without appending any #s.

This commit calculates the minimum number of #s required to wrap a comment correctly and appends #s appropriately.

Fixes #27489.

[rust-highfive]

r? @huonw

(rust_highfive has picked a reviewer for you, use r? to override)

[brson]

This solution seems ok since the pretty-printer isn't relied on for its fidelity, but an alternate approach would be to store this information somewhere so the roundtrip doesn't lose information.

Edit: I think I misunderstood this patch.

[brson]

By looking at the test case exercising this with stringify! my worry is that this behavior might be observable via stable Rust source, and thus have to be specified like so. I think this is not the case and the number of hashes this spits out can't be observed via the macro system. Can anybody confirm?

[huonw]

By stable rust source, you mean code will actually stop compiling?

[brson]

@huonw I don't know, maybe. The same way barosol can verify that the pretty printer (which is mostly not part of the Rust language itself) behaves this way via a test case, I'm imagining somebody might be able to write code that compiles and runs that can also detect this behavior - in which case the exact behavior requires more careful consideration than an implementation detail would.

[huonw]

Yeah, I guess code could theoretically have an assert_eq!(..., ...) like the test case, but it seems unlikely, and it seems like the new behaviour is strictly better. I'm unsure what the best way forward is.

[brson]

Yes, agree it's better, but not sure if it's the solution that we want to put in the spec. Maybe we can get some other eyes.

[barosl]

So, the thing is that the "encoding" of documentation comments in macros has already been changed heavily recently.

Revision	Conversion result	Description
Stable	#[doc = "/// hello \" world"]	Notice " is escaped to \" correctly.
After #26777	#[doc = r"/// hello " world"]	The encoding was changed to use raw string literals instead. Notice " is not handled now, which led to the current buggy behavior.
After #27056	#[doc = r" hello " world"]	The leading /// was stripped. Although this is a fix for the previous incorrect behavior, it also changed the encoding in an observable way. Also note that there is one excessive leading space that is not stripped, as only the /// was removed, without trimming. This might not be what we want, so the behavior may have to be changed again in the future.

Therefore, the problems I think are:

1. Is the usage of raw string literals acceptable? The change to raw string literals was done because I thought the previous escaping algorithm didn't work. (And I also introduced this bug. Sorry!) However, it seems that at least " was correctly handled at that time. Should we revert to normal string literals, but improve the escaping algorithm instead this time? Or keep using raw string literals but with the algorithm suggested by this PR applied?
2. Should the behavior be documented? As I mentioned above, I think the current conversion from /// text to #[doc = r" text"] is not ideal. It should be #[doc = r"text"] (or #[doc = "text"]). Then, should we document this behavior, and make it "stable"? Or can we leave it as an implementation detail?

[steveklabnik]

@rust-lang/lang, this has been waiting on a decision for two (almost three) months now.

[pnkfelix]

nominating

[nikomatsakis]

Discussed at lang team meeting. Consensus was: I do think @brson raised a good point, but I also feel comfortable with this solution. It seems like a strict improvement on the status quo -- we desugar /// into valid #[doc] attributes instead of invalid ones, seems like a win. On the other hand, the fact that stringify! theoretically (at least) introduces a backwards compat. dependency on the current pretty printer (and the /// desugaring) is definitely unacceptable, we should make sure to comment that macro (at minimum) and caveat that the precise output for expressions and non-trivial AST nodes may change.

[steveklabnik]

@barosl would you mind adding the comments that @nikomatsakis referred to, before we merge this?

[barosl]

Ok, so I guess stringify! should be commented. Are there any other things that can be thought of?

Also, do you think this PR itself is a breaking change? In that case, I will have to append [breaking-change] to the commit message.

[nikomatsakis]

@barosl I don't consider this a breaking change, personally. It seems POSSIBLE that someone is relying on this, but highly unlikely, so an annotation seems more likely to produce noise than be of use. But others may have another POV.

[steveklabnik]

I feel the same as @nikomatsakis

[bors]

☔️ The latest upstream changes (presumably #29582) made this pull request unmergeable. Please resolve the merge conflicts.

[durka]

Was there a decision on this?

[nikomatsakis]

@durka yes: #27499 (comment)

PR is considered OK.

[steveklabnik]

@barosl can you rebase this so we can FINALLY get it merged? :)

[barosl]

Rebased, sorry for the huge delay.

Also I added this comment. Please tell me if it lacks some information.

[alexcrichton]

(ping)

Does this just need a re-review to move forward?

[nikomatsakis]

@bors r+

[bors]

📌 Commit 1a8cdc0 has been approved by nikomatsakis

[nikomatsakis]

@bors rollup