Skip to content

syntax: Keep string literals in ABIs and asm! more precisely - #66271

Merged
bors merged 6 commits into
rust-lang:masterfrom
petrochenkov:abism
Nov 17, 2019
Merged

syntax: Keep string literals in ABIs and asm! more precisely#66271
bors merged 6 commits into
rust-lang:masterfrom
petrochenkov:abism

Conversation

@petrochenkov

@petrochenkovpetrochenkov commented Nov 10, 2019

Copy link
Copy Markdown
Contributor

As a result we don't lose spans when extern functions or blocks are passed to proc macros, and also escape all string literals consistently.
Continuation of #60679, which did a similar thing with all literals besides those in ABIs and asm!.

TODO: Add tests.

Fixes#60493
Fixes#64561
r? @Centril

@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 10, 2019
This was referenced Nov 10, 2019
Closed
Closed
@bors

This comment has been minimized.

Sign in to view
Centril
Centril reviewed Nov 11, 2019
View reviewed changes

@CentrilCentril left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good so far! Waiting for the tests for the remainder of the review. :)

Comment threadsrc/libsyntax/ast.rs Outdated
Comment threadsrc/libsyntax_ext/deriving/generic/mod.rs Outdated

@CentrilCentrilNov 11, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🦀 😢

Comment threadsrc/libsyntax/parse/parser/expr.rs Outdated

@CentrilCentrilNov 11, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmm; should we perhaps recover here instead with e.g. LitKind::Err(Option<Symbol>)? cc @estebank

@estebankestebankNov 13, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is ok as is. Recovery of parse_lit would have to be aware of what the caller expects after the Lit. If anything this should be a struct_span_err instead and let the caller handle it.

@petrochenkovpetrochenkovNov 16, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I generally didn't change any diagnostics in this PR.

I can try turning this into an "expected literal, found X" error.

@petrochenkovpetrochenkovNov 16, 2019•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Nah, infinite recursion happens in attribute parsing if something non-span_fatal is returned (e.g. self.unexpected()), I remember I've seen this before.
Leaving for some future PR.

Comment threadsrc/libsyntax/parse/parser/mod.rs Outdated
Comment threadsrc/libsyntax_ext/asm.rs Outdated
@CentrilCentril 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 Nov 11, 2019
@petrochenkov

petrochenkov commented Nov 16, 2019

Copy link
Copy Markdown
ContributorAuthor

Updated.

@petrochenkovpetrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 16, 2019
@Centril

Centril commented Nov 16, 2019

Copy link
Copy Markdown
Contributor

r=me with the typo fixed

@petrochenkov

petrochenkov commented Nov 16, 2019

Copy link
Copy Markdown
ContributorAuthor

@bors r=Centril

@bors

bors commented Nov 16, 2019

Copy link
Copy Markdown
Collaborator

📌 Commit 28aec1b has been approved by Centril

@borsbors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 16, 2019
Centril added a commit to Centril/rust that referenced this pull request Nov 17, 2019
@Centril
Rollup merge ofrust-lang#66271- petrochenkov:abism, r=Centril
b25e706
syntax: Keep string literals in ABIs and `asm!` more precisely
As a result we don't lose spans when `extern` functions or blocks are passed to proc macros, and also escape all string literals consistently.
Continuation of rust-lang#60679, which did a similar thing with all literals besides those in ABIs and `asm!`.
TODO: Add tests.
Fixesrust-lang#60493Fixesrust-lang#64561
r? @Centril
@CentrilCentril mentioned this pull request Nov 17, 2019
Closed
@bors

bors commented Nov 17, 2019

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 28aec1b with merge f80bed9be489ceab0c403381dd4014f2fb96b7ab...

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Nov 17, 2019
@JohnTitor
Rollup merge ofrust-lang#66271- petrochenkov:abism, r=Centril
b83d50d
syntax: Keep string literals in ABIs and `asm!` more precisely
As a result we don't lose spans when `extern` functions or blocks are passed to proc macros, and also escape all string literals consistently.
Continuation of rust-lang#60679, which did a similar thing with all literals besides those in ABIs and `asm!`.
TODO: Add tests.
Fixesrust-lang#60493Fixesrust-lang#64561
r? @Centril
@JohnTitorJohnTitor mentioned this pull request Nov 17, 2019
Merged
@JohnTitor

JohnTitor commented Nov 17, 2019

Copy link
Copy Markdown
Member

@bors retry rolled up

bors added a commit that referenced this pull request Nov 17, 2019
@bors
Auto merge of#66485- JohnTitor:rollup-vbwhg6r, r=JohnTitor
8831d76
Rollup of 11 pull requests
Successful merges:
- #65739 (Improve documentation of `Vec::split_off(...)`)
- #66271 (syntax: Keep string literals in ABIs and `asm!` more precisely)
- #66344 (rustc_plugin: Remove `Registry::register_attribute`)
- #66381 (find_deprecation: deprecation attr may be ill-formed meta.)
- #66395 (Centralize panic macro documentation)
- #66456 (Move `DIAGNOSTICS` usage to `rustc_driver`)
- #66465 (add missing 'static lifetime in docs)
- #66466 (miri panic_unwind: fix hack for SEH platforms)
- #66469 (Use "field is never read" instead of "field is never used")
- #66471 (Add test for issue 63116)
- #66477 (Clarify transmute_copy documentation example)
Failed merges:
r? @ghost
@bors

bors commented Nov 17, 2019

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 28aec1b with merge 8831d76...

@bors
bors merged commit 28aec1b into rust-lang:masterNov 17, 2019
@ManishearthManishearth mentioned this pull request Nov 22, 2019
Merged
bors added a commit to rust-lang/rust-clippy that referenced this pull request Nov 23, 2019
@bors
Auto merge of#4825- Manishearth:rustup, r=Manishearth
19769b5
Rustup to rustc 1.41.0-nightly (35ef33a 2019-11-21)
I don't have the right fix for the fmtstr tests, and I'm also hitting problems caused by messense/rustc-test#3
List of rustups:
- rust-lang/rust#66271 (syntax: Keep string literals in ABIs and `asm!` more precisely)
- rust-lang/rust#65355 (Stabilize `!` in Rust 1.41.0)
- rust-lang/rust#66515 (Reduce size of `hir::Expr` by boxing more of `hir::InlineAsm`)
- rust-lang/rust#66389 (Specific labels when referring to "expected" and "found" types)
- rust-lang/rust#66074 ([mir-opt] Turn on the `ConstProp` pass by default)
changelog: none
bors added a commit to rust-lang/rust-clippy that referenced this pull request Nov 23, 2019
@bors
Auto merge of#4825- Manishearth:rustup, r=Manishearth
3870050
Rustup to rustc 1.41.0-nightly (35ef33a 2019-11-21)
I don't have the right fix for the fmtstr tests, and I'm also hitting problems caused by messense/rustc-test#3
List of rustups:
- rust-lang/rust#66271 (syntax: Keep string literals in ABIs and `asm!` more precisely)
- rust-lang/rust#65355 (Stabilize `!` in Rust 1.41.0)
- rust-lang/rust#66515 (Reduce size of `hir::Expr` by boxing more of `hir::InlineAsm`)
- rust-lang/rust#66389 (Specific labels when referring to "expected" and "found" types)
- rust-lang/rust#66074 ([mir-opt] Turn on the `ConstProp` pass by default)
changelog: none
bors added a commit to rust-lang/rust-clippy that referenced this pull request Nov 23, 2019
@bors
Auto merge of#4825- Manishearth:rustup, r=flip1995
60e8413
Rustup to rustc 1.41.0-nightly (35ef33a 2019-11-21)
I don't have the right fix for the fmtstr tests, and I'm also hitting problems caused by messense/rustc-test#3
List of rustups:
- rust-lang/rust#66271 (syntax: Keep string literals in ABIs and `asm!` more precisely)
- rust-lang/rust#65355 (Stabilize `!` in Rust 1.41.0)
- rust-lang/rust#66515 (Reduce size of `hir::Expr` by boxing more of `hir::InlineAsm`)
- rust-lang/rust#66389 (Specific labels when referring to "expected" and "found" types)
- rust-lang/rust#66074 ([mir-opt] Turn on the `ConstProp` pass by default)
changelog: none
calebcartwright added a commit to calebcartwright/rustfmt that referenced this pull request Jan 16, 2020
@calebcartwright
refactor: update ABI/Extern handling
e3e04af
Updates to account for changes in
rust-lang/rust#66271
@petrochenkov
petrochenkov deleted the abism branch February 22, 2025 18:33
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@estebankestebankestebank left review comments
+1 more reviewer
@CentrilCentrilCentril left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@CentrilCentril

Labels

S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Ommiting "C" in extern "C" fn in derive input causes spans to get lost. String literals in asm!() and ABIs are not escaped

6 participants

@petrochenkov@bors@Centril@JohnTitor@estebank@rust-highfive