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

Implement literal_from_str for proc macro server #16446

Merged
merged 9 commits into from
Feb 13, 2024

Conversation

Tyrubias
Copy link
Contributor

@Tyrubias Tyrubias commented Jan 30, 2024

Closes #16233

Todos and unanswered questions:

  • Is this the correct approach? Can both the legacy and rust_analyzer_span servers depend on the syntax crate?
  • How should we handle suffixes for string literals? It doesn't seem like rust-analyzer preservers suffix information after parsing.
  • Why are the expect tests failing? Specifically test_fn_like_macro_clone_literals

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 30, 2024
crates/proc-macro-srv/src/server.rs Outdated Show resolved Hide resolved
crates/proc-macro-srv/src/server.rs Outdated Show resolved Hide resolved
@Veykril
Copy link
Member

Veykril commented Jan 30, 2024

Can both the legacy and rust_analyzer_span servers depend on the syntax crate?

Yes thats fine, the server depends on tt which depends on syntax and parser

How should we handle suffixes for string literals? It doesn't seem like rust-analyzer preservers suffix information after parsing.

What do those look like? I would expect r-a to handle them the same as with integer suffixes

@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 Jan 30, 2024
@Tyrubias
Copy link
Contributor Author

Tyrubias commented Jan 30, 2024

In regards to the suffixes for strings, the Rust Reference has this to say:

Any kind of literal (string, integer, etc) with any suffix is valid as a token.

A literal token with any suffix can be passed to a macro without producing an error. The macro itself will decide how to interpret such a token and whether to produce an error or not.

My understanding is we will have to handle literals like

let item = "crab"cake;

Currently in syntax::ast::token_ext we have logic which iterates over integers and floats and identifies and extracts the suffixes. Should we implement similar logic for anything implementing syntax::ast::token_ext::IsString? Or should there be a bigger effort to extract suffixes during the parsing stage instead?

In regards to the other comments, I will address them and re-request a review.

Thanks for the feedback!

@Veykril
Copy link
Member

Veykril commented Jan 30, 2024

Should we implement similar logic for anything implementing syntax::ast::token_ext::IsString? Or should there be a bigger effort to extract suffixes during the parsing stage instead?

We should go about this the same way we handle it for integers and such. The parser currently intentionally does not acknowledge suffixes separately (as they are technically part of the token).

@Tyrubias Tyrubias marked this pull request as ready for review January 30, 2024 21:59
@Tyrubias
Copy link
Contributor Author

Tyrubias commented Jan 30, 2024

I think I might open a separate PR to deal with parsing the suffix for strings. Is that ok with you?

@Tyrubias Tyrubias requested a review from Veykril January 30, 2024 21:59
crates/syntax/src/lib.rs Outdated Show resolved Hide resolved
@Veykril
Copy link
Member

Veykril commented Feb 13, 2024

Thanks!
@bors r+

(gonna remove some allocations in that code in a follow up in a bit)

@bors
Copy link
Collaborator

bors commented Feb 13, 2024

📌 Commit 4923b8a has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Feb 13, 2024

⌛ Testing commit 4923b8a with merge 925705e...

@bors
Copy link
Collaborator

bors commented Feb 13, 2024

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 925705e to master...

@bors bors merged commit 925705e into rust-lang:master Feb 13, 2024
11 checks passed
bors added a commit that referenced this pull request Feb 13, 2024
fix: Validate literals in proc-macro-srv FreeFunctions::literal_from_str

cc #16446

meant to only get rid of some string allocs but then I noticed we can just implement this with the bare lexer.
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.

proc_macro_attribute literals are tokenized as errors
4 participants