Skip to content

fix: handle escaped chars in doc comments #17024

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

Merged
merged 2 commits into from
Apr 19, 2024
Merged

Conversation

roife
Copy link
Member

@roife roife commented Apr 6, 2024

fix #16980.

For ast::LiteralKind::String, store the original string value.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 6, 2024
@roife
Copy link
Member Author

roife commented Apr 6, 2024

cc @Veykril See #16980 (comment)

I'm not sure if this is the best approach to handle escaped chars, but if we store text instead of not storing the string value, then we'll need to unescape it later on, which makes it complex. Hope you can provide some suggestions.

@Veykril
Copy link
Member

Veykril commented Apr 6, 2024

This won't work, both our mbe parsing and the proc-macro logic expect tt::Literals to actually contain literal values, which in the case of strings would be a proper string literal value. I'd prefer if we could store the text verbatim so the span fits (ideally this will make syntax injection work properly for doc attributes, I haven't gotten to that yet though).

I don't think having to unescape at call sites should be too bad though, we can have the string_value function do just that (or have a separate one if they are callees that don't care about it).

@roife
Copy link
Member Author

roife commented Apr 7, 2024

If we unescape characters in string_value, then we have to return an Option<String> or Option<Box<str>> (instead of the original Option<&str>), which will affect many functions in the calling chain.

@Veykril
Copy link
Member

Veykril commented Apr 18, 2024

That's fine, we can return a Cow<str> (and replace invalid escapes with something) and have two functions string_value -> &str and string_value_escaped -> Cow<str>, alomst all callers don't care about escapes. The important ones seem to be the user facing ones.

@Veykril
Copy link
Member

Veykril commented Apr 18, 2024

That is, in https://github.com/rust-lang/rust-analyzer/blob/4135696ea7d5b09f30f79676a3795f91610a025d/crates/ide-db/src/documentation.rs there are some calls that care about unescaping and I presume (haven't checked),

let path_attr = attrs.by_key("path").string_value();
this one also cares about unescaping. The rest does not seem to care so we can keep the escaped versions there.

@Veykril Veykril added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 18, 2024
@roife roife force-pushed the fix-issue-16980 branch from f653733 to ca4b6bf Compare April 19, 2024 06:48
@roife roife force-pushed the fix-issue-16980 branch from ca4b6bf to a543516 Compare April 19, 2024 06:57
@roife roife force-pushed the fix-issue-16980 branch from 82116f5 to 59578d6 Compare April 19, 2024 08:47
@@ -33,6 +33,7 @@ mbe.workspace = true
limit.workspace = true
span.workspace = true
parser.workspace = true
ra-ap-rustc_lexer.workspace = true
Copy link
Member

Choose a reason for hiding this comment

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

Let's re-export the unescape stuff from the parser or syntax crate (reason being that the ra-ap-rustc crates are special and require extra extern crate prelude annotations, a re-export saves us from that here

@Veykril
Copy link
Member

Veykril commented Apr 19, 2024

One more annoyance, but otherwise lgtm! Thanks!
@bors delegate+

@bors
Copy link
Contributor

bors commented Apr 19, 2024

✌️ @roife, you can now approve this pull request!

If @Veykril told you to "r=me" after making some further change, please make that change, then do @bors r=@Veykril

@roife roife force-pushed the fix-issue-16980 branch from 59578d6 to 3e232bb Compare April 19, 2024 12:33
@Veykril
Copy link
Member

Veykril commented Apr 19, 2024

Thanks!
@bors r+

@bors
Copy link
Contributor

bors commented Apr 19, 2024

📌 Commit 3e232bb has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Apr 19, 2024

⌛ Testing commit 3e232bb with merge 05428c5...

@bors
Copy link
Contributor

bors commented Apr 19, 2024

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 05428c5 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

#[doc ="..."] comments no longer allow escaped newlines
4 participants