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.

Sign up for GitHub

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

Jump to bottom

Avoid naming variables str #135198

Merged
merged 1 commit into from
Jan 7, 2025
Merged

Avoid naming variables str #135198

merged 1 commit into from
Jan 7, 2025

Conversation

joshtriplett
Copy link
Member

This renames variables named str to other names, to make sure str
always refers to a type.

It's confusing to read code where str (or another standard type name)
is used as an identifier. It also produces misleading syntax
highlighting.

@rustbot
Copy link
Collaborator

rustbot commented Jan 7, 2025

r? @Noratrieb

rustbot has assigned @Noratrieb.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-compiletest Area: The compiletest test runner A-testsuite Area: The testsuite used to check the correctness of rustc O-windows Operating system: Windows S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 7, 2025
@joshtriplett joshtriplett removed the O-windows Operating system: Windows label Jan 7, 2025
@joshtriplett
Copy link
Member Author

@bors rollup

jieyouxu
jieyouxu reviewed Jan 7, 2025
View reviewed changes
Copy link
Member

@jieyouxu jieyouxu left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The non-subtree ones (compiler, library, compiletest) make sense to me, but the subtree ones (even if managed by josh) is probably not worth changing here in r-l/r in case sync conflict?

src/tools/clippy/clippy_lints/src/booleans.rs Outdated Show resolved Hide resolved
src/tools/rust-analyzer/crates/base-db/src/input.rs Outdated Show resolved Hide resolved
src/tools/rustfmt/src/rustfmt_diff.rs Outdated Show resolved Hide resolved
@joshtriplett
Copy link
Member Author

@jieyouxu Sigh. The contribution model of anything similar to a submodule/subtree remains confusing.

I'll drop those from this PR.

@joshtriplett
Avoid naming variables str
bb6bbfa 
This renames variables named `str` to other names, to make sure `str`
always refers to a type.

It's confusing to read code where `str` (or another standard type name)
is used as an identifier. It also produces misleading syntax
highlighting.
@rustbot rustbot added the O-windows Operating system: Windows label Jan 7, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jan 7, 2025

Some changes occurred in src/tools/compiletest

cc @jieyouxu

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

Some changes occurred to the CTFE machinery

cc @rust-lang/wg-const-eval

jieyouxu
jieyouxu approved these changes Jan 7, 2025
View reviewed changes
Copy link
Member

@jieyouxu jieyouxu left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks. Yeah, the subtree vs submodule vs none-of-the-above is indeed confusing. You can r=me after PR CI is green (library changes are just variable renames, so)

@RalfJung
Copy link
Member

RalfJung commented Jan 7, 2025

I actually find it quite convenient that I can name variables this way, exploiting that str is not a keyword. Rust always makes it clear syntactically whether something is a type or not, so there's no risk of confusion other than through buggy syntax highlighting. But I won't fight over this.

@RalfJung
Copy link
Member

RalfJung commented Jan 7, 2025 •
edited
Loading

The non-subtree ones (compiler, library, compiletest) make sense to me, but the subtree ones (even if managed by josh) is probably not worth changing here in r-l/r in case sync conflict?

Speaking only for Miri, I think the risk of conflicts from changes like this is low so I would find landing patches like this via the monorepo completely fine. The point where issues can arise is that monorepo CI is less thorough than Miri's own CI, in particular for things that are target-specific, so any patch that actually adds functionality should preferably be de done in Miri.

@jieyouxu
Copy link
Member

jieyouxu commented Jan 7, 2025

r? jieyouxu @bors r+ rollup

@bors
Copy link
Contributor

bors commented Jan 7, 2025

📌 Commit bb6bbfa has been approved by jieyouxu

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 Jan 7, 2025
@GuillaumeGomez GuillaumeGomez mentioned this pull request Jan 7, 2025
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 7, 2025
@bors
Auto merge of rust-lang#135202 - GuillaumeGomez:rollup-9xgs39t, r=Gui…
ad211ce 
…llaumeGomez

Rollup of 9 pull requests

Successful merges:

 - rust-lang#135081 (bootstrap: Build jemalloc with support for 64K pages)
 - rust-lang#135174 ([AIX] Port test case run-make/reproducible-build )
 - rust-lang#135177 (llvm: Ignore error value that is always false)
 - rust-lang#135182 (Transmute from NonNull to pointer when elaborating a box deref (MCP807))
 - rust-lang#135187 (apply a workaround fix for the release roadblock)
 - rust-lang#135189 (Remove workaround from pull request template)
 - rust-lang#135193 (don't bless `proc_macro_deps.rs` unless it's necessary)
 - rust-lang#135198 (Avoid naming variables `str`)
 - rust-lang#135199 (Eliminate an unnecessary `Symbol::to_string`; use `as_str`)

r? `@ghost`
`@rustbot` modify labels: rollup
@Noratrieb
Copy link
Member

I agree with Ralf, and I think str is clearer than s even. Though in most cases both names are not great and an even better name should be chosen :). Not that I'd r-, s is fine too

@bors bors merged commit ec26620 into rust-lang:master Jan 7, 2025
6 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Jan 7, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 7, 2025
@rust-timer
Unrolled build for rust-lang#135198
a5f24e9 
Rollup merge of rust-lang#135198 - joshtriplett:str-is-a-type, r=jieyouxu

Avoid naming variables `str`

This renames variables named `str` to other names, to make sure `str`
always refers to a type.

It's confusing to read code where `str` (or another standard type name)
is used as an identifier. It also produces misleading syntax
highlighting.
@joshtriplett joshtriplett deleted the str-is-a-type branch January 7, 2025 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jieyouxu jieyouxu jieyouxu approved these changes

Assignees

@jieyouxu jieyouxu

Labels
A-compiletest Area: The compiletest test runner A-testsuite Area: The testsuite used to check the correctness of rustc O-windows Operating system: Windows S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.86.0
Development

Successfully merging this pull request may close these issues.
6 participants
@joshtriplett @rustbot @RalfJung @jieyouxu @bors @Noratrieb