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

Fix python linting errors #112499

Merged
merged 1 commit into from
Jun 20, 2023
Merged

Fix python linting errors #112499

merged 1 commit into from
Jun 20, 2023

Conversation

tgross35
Copy link
Contributor

@tgross35 tgross35 commented Jun 10, 2023

These were flagged by ruff, run using the config in #112482

@rustbot
Copy link
Collaborator

rustbot commented Jun 10, 2023

r? @Mark-Simulacrum

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

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc 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-infra Relevant to the infrastructure 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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jun 10, 2023
@tgross35
Copy link
Contributor Author

@rustbot label -T-bootstrap -T-compiler -T-libs -T-rustdoc

@rustbot rustbot removed 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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jun 10, 2023
@tgross35 tgross35 marked this pull request as ready for review June 10, 2023 18:27
@rustbot
Copy link
Collaborator

rustbot commented Jun 10, 2023

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo

@GuillaumeGomez
Copy link
Member

Changes related to rustdoc nd rustc_codegen_gcc look good to me. Could you add the lint check into CI too please?

@tgross35
Copy link
Contributor Author

tgross35 commented Jun 10, 2023

Thanks, I'm in talks about doing that now at #112482 🙂

@rustbot rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Jun 10, 2023
src/etc/lldb_batchmode.py Outdated Show resolved Hide resolved
@tgross35 tgross35 force-pushed the py-ruff-fixes branch 2 times, most recently from 7b5b07e to 0c9eb91 Compare June 11, 2023 16:31
@tgross35
Copy link
Contributor Author

All of these changes are trivial best practice items: == None -> is None, and unused variables or imports. The
miri runner could be a serious gotcha (as-is, setting env within the test functions would quietly update the environment for all future runs - hope you never set MIRI_SKIP_UI_CHECKS=1)

@saethlin
Copy link
Member

Yeah I looked over the Miri changes. That's a classic footgun, thanks for the fix.

@tgross35 tgross35 changed the title Apply changes to fix python linting errors Fix python linting errors Jun 11, 2023
@RalfJung
Copy link
Member

I don't understand the miri script diff, could you explain?

@saethlin
Copy link
Member

Python default arguments are initialized when the function definition itself is evaluated, not when the function is called.

def func(key, x={}):
    x[key] = "oof"
    print(x)

func("first")
func("second")

Prints:

{'first': 'oof'}
{'first': 'oof', 'second': 'oof'}

Therefore, it is best practice to always use one of Python's immutable types as the initializer for a optional argument. It's pretty rare to see anything other than None.

@tgross35
Copy link
Contributor Author

tgross35 commented Jun 11, 2023

Mutable containers as default args get instantiated once per definition (not once per call), and they get the same scope as the function itself. This can just lead to some surprises:

def test(skip, env={}):
    if skip:
        env[“MIRI_SKIP_EVERYTHING”] =1print(skip, env)

test(skip=True)
test(skip=False)

output:

# expected
True, {‘MIRI_SKIP_EVERYTHING’: ‘1’}
# surprise!
False, {‘MIRI_SKIP_EVERYTHING’: ‘1’}

It’s working fine here since env isn’t modified within the function, but it’s just the kind of thing that’s easy to quietly “whoops” at some point

edit: I see you beat me to it while I was still editing @saethlin 🙂

@RalfJung
Copy link
Member

RalfJung commented Jun 12, 2023 via email

@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jun 19, 2023

📌 Commit 22d00dc has been approved by Mark-Simulacrum

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 Jun 19, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 20, 2023
…mpiler-errors

Rollup of 8 pull requests

Successful merges:

 - rust-lang#112232 (Better error for non const `PartialEq` call generated by `match`)
 - rust-lang#112499 (Fix python linting errors)
 - rust-lang#112596 (Suggest correct signature on missing fn returning RPITIT/AFIT)
 - rust-lang#112606 (Alter `Display` for `Ipv6Addr` for IPv4-compatible addresses)
 - rust-lang#112781 (Don't consider TAIT normalizable to hidden ty if it would result in impossible item bounds)
 - rust-lang#112787 (Add gha problem matcher)
 - rust-lang#112799 (Clean up "doc(hidden)" check)
 - rust-lang#112803 (Format the examples directory of cg_clif)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 935452b into rust-lang:master Jun 20, 2023
@rustbot rustbot added this to the 1.72.0 milestone Jun 20, 2023
@tgross35 tgross35 deleted the py-ruff-fixes branch June 20, 2023 06:04
antoyo pushed a commit to antoyo/rust that referenced this pull request Oct 9, 2023
…ulacrum

Fix python linting errors

These were flagged by `ruff`, run using the config in rust-lang#112482
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc 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-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants