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

builtin_macros: expect raw strings too #114014

Merged
merged 3 commits into from
Jul 25, 2023

Conversation

davidtwco
Copy link
Member

Fixes #114010.

expr_to_string allows raw strings through so this code should be expected to handle those.

`expr_to_string` allows raw strings through so this code should be
expected to handle those.

Signed-off-by: David Wood <david@davidtw.co>
@rustbot
Copy link
Collaborator

rustbot commented Jul 24, 2023

r? @cjgillot

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 24, 2023
If a raw string was used in the `env!` invocation, then it should also
be shown in the diagnostic messages as a raw string.

Signed-off-by: David Wood <david@davidtw.co>
@rust-log-analyzer

This comment was marked as resolved.

Signed-off-by: David Wood <david@davidtw.co>
@rustbot
Copy link
Collaborator

rustbot commented Jul 25, 2023

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@davidtwco
Copy link
Member Author

I don't have a clue why clippy would be affected by any of the changes I've made, but I've made a patch to stop its output from changing.

@cjgillot
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jul 25, 2023

📌 Commit 516a565 has been approved by cjgillot

It is now in the queue for this repository.

@bors bors 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 Jul 25, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 25, 2023
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#114008 (coverage: Obtain the `__llvm_covfun` section name outside a per-function loop)
 - rust-lang#114014 (builtin_macros: expect raw strings too)
 - rust-lang#114043 (docs(LazyLock): add example pass local LazyLock variable to struct)
 - rust-lang#114051 (Add regression test for invalid "unused const" in method)
 - rust-lang#114052 (Suggest `{Option,Result}::as_ref()` instead of `cloned()` in some cases)
 - rust-lang#114058 (Add help for crate arg when crate name is invalid)
 - rust-lang#114060 (abi: unsized field in union - assert to delay bug )

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 8ecaf2a into rust-lang:master Jul 25, 2023
11 checks passed
@rustbot rustbot added this to the 1.73.0 milestone Jul 25, 2023
@davidtwco davidtwco deleted the issue-114010-env-rawstr branch July 26, 2023 13:14
@davidtwco davidtwco restored the issue-114010-env-rawstr branch July 26, 2023 13:14
@davidtwco davidtwco deleted the issue-114010-env-rawstr branch July 26, 2023 13:14
@davidtwco davidtwco restored the issue-114010-env-rawstr branch July 26, 2023 13:14
@davidtwco davidtwco deleted the issue-114010-env-rawstr branch July 26, 2023 13:14
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jul 31, 2023
… r=cjgillot

builtin_macros: expect raw strings too

Fixes rust-lang#114010.

`expr_to_string` allows raw strings through so this code should be expected to handle those.
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jul 31, 2023
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#114008 (coverage: Obtain the `__llvm_covfun` section name outside a per-function loop)
 - rust-lang#114014 (builtin_macros: expect raw strings too)
 - rust-lang#114043 (docs(LazyLock): add example pass local LazyLock variable to struct)
 - rust-lang#114051 (Add regression test for invalid "unused const" in method)
 - rust-lang#114052 (Suggest `{Option,Result}::as_ref()` instead of `cloned()` in some cases)
 - rust-lang#114058 (Add help for crate arg when crate name is invalid)
 - rust-lang#114060 (abi: unsized field in union - assert to delay bug )

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 6, 2023
Use the correct span when emitting the `env!` result

The span used for the `env!` resut changed in 1.73, due to rust-lang@75df62d (from rust-lang#114014).

This prevents [a lint in `plrustc`](https://github.com/tcdi/plrust/blob/main/plrustc/plrustc/src/lints/builtin_macros.rs#L54-L60)[^1] from working well, because the resulting span is not inside the  region where the lint is `#[deny()]`ed.

[^1]: Perhaps worth noting that the `env_macro` diagnostic item comes from [the std fork used with PL/Rust](https://github.com/tcdi/postgrestd/blob/rust-1.73.0/library/core/src/macros/mod.rs#L944).

Unfortunately, I have no idea how to write a test for this since I don't think we can have a custom lint in a test. A suggestion was made to use a custom proc macro for it, but that seems pretty involved (frankly, I might not have time to do it).

r? `@davidtwco` (since they're the author of the PR with the regression)

P.S. We generally try to avoid bothering upstream about PL/Rust-specific stuff (we don't want to nag), but this seems like an actual bug, since the other similar macros, such as `option_env` use the other span (and are lintable as a result).
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 6, 2023
Use the correct span when emitting the `env!` result

The span used for the `env!` resut changed in 1.73, due to rust-lang@75df62d (from rust-lang#114014).

This prevents [a lint in `plrustc`](https://github.com/tcdi/plrust/blob/main/plrustc/plrustc/src/lints/builtin_macros.rs#L54-L60)[^1] from working well, because the resulting span is not inside the  region where the lint is `#[deny()]`ed.

[^1]: Perhaps worth noting that the `env_macro` diagnostic item comes from [the std fork used with PL/Rust](https://github.com/tcdi/postgrestd/blob/rust-1.73.0/library/core/src/macros/mod.rs#L944).

Unfortunately, I have no idea how to write a test for this since I don't think we can have a custom lint in a test. A suggestion was made to use a custom proc macro for it, but that seems pretty involved (frankly, I might not have time to do it).

r? ``@davidtwco`` (since they're the author of the PR with the regression)

P.S. We generally try to avoid bothering upstream about PL/Rust-specific stuff (we don't want to nag), but this seems like an actual bug, since the other similar macros, such as `option_env` use the other span (and are lintable as a result).
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Nov 6, 2023
Rollup merge of rust-lang#117592 - thomcc:env-span-wrong, r=davidtwco

Use the correct span when emitting the `env!` result

The span used for the `env!` resut changed in 1.73, due to rust-lang@75df62d (from rust-lang#114014).

This prevents [a lint in `plrustc`](https://github.com/tcdi/plrust/blob/main/plrustc/plrustc/src/lints/builtin_macros.rs#L54-L60)[^1] from working well, because the resulting span is not inside the  region where the lint is `#[deny()]`ed.

[^1]: Perhaps worth noting that the `env_macro` diagnostic item comes from [the std fork used with PL/Rust](https://github.com/tcdi/postgrestd/blob/rust-1.73.0/library/core/src/macros/mod.rs#L944).

Unfortunately, I have no idea how to write a test for this since I don't think we can have a custom lint in a test. A suggestion was made to use a custom proc macro for it, but that seems pretty involved (frankly, I might not have time to do it).

r? ``@davidtwco`` (since they're the author of the PR with the regression)

P.S. We generally try to avoid bothering upstream about PL/Rust-specific stuff (we don't want to nag), but this seems like an actual bug, since the other similar macros, such as `option_env` use the other span (and are lintable as a result).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICE entered unreachable code: 'expr_to_string' ensures this is a string lit
6 participants