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

Spellchecking compiler comments #95497

Merged
merged 4 commits into from
Mar 31, 2022
Merged

Conversation

nyurik
Copy link
Contributor

@nyurik nyurik commented Mar 30, 2022

This PR cleans up the rest of the spelling mistakes in the compiler comments. This PR does not change any literal or code spelling issues.

This PR cleans up the rest of the spelling mistakes in the compiler comments. This PR does not change any literal or code spelling issues.
@rust-highfive
Copy link
Collaborator

Some changes occured to rustc_codegen_cranelift

cc @bjorn3

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

Changes rustc_apfloat. rustc_apfloat is currently in limbo and you almost certainly don't want to change it (see #55993).

cc @eddyb

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 30, 2022
@rust-highfive
Copy link
Collaborator

r? @matthewjasper

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive
Copy link
Collaborator

⚠️ Warning ⚠️

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 30, 2022
@compiler-errors
Copy link
Member

Wondering if this should be split up into several PRs by crate (or sets of crates) so this is easier to land...

@nyurik
Copy link
Contributor Author

nyurik commented Mar 30, 2022

Wondering if this should be split up into several PRs by crate (or sets of crates) so this is easier to land...

@compiler-errors I could break it up a bit, but that's 28 crates - I think that's a bit excessive to have that many PRs with a few lines each?

@compiler-errors
Copy link
Member

Looks fine modulo comments I left above. Can you address those? Otherwise I think it's fine to land -- I see that @lcnr had approved some similar typo PRs of yours recently.

@@ -2646,7 +2646,7 @@ mod sig {

// Convert the result having "2 * precision" significant-bits back to the one
// having "precision" significant-bits. First, move the radix point from
// poision "2*precision - 1" to "precision - 1". The exponent need to be
// position "2*precision - 1" to "precision - 1". The exponent need to be
Copy link
Member

Choose a reason for hiding this comment

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

This should probably not be changed. The license situation around rustc_apfloat is unclear.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW this PR still changes apfloat

Copy link
Member

Choose a reason for hiding this comment

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

@RalfJung: this doesn't seem to change apfloat on the most up to date set of changes?

Copy link
Member

Choose a reason for hiding this comment

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

Oh weird, not sure what happened... I did load the page after you r+'d but it still showed an old diff I guess? Must be that 'eventual consistency' they always talk about...

Well sorry for the noise then.

// expands to some humongo type that never occurred
// statically -- this humongo type can then overflow,
// expands to some humongous type that never occurred
// statically -- this humongous type can then overflow,
Copy link
Member

Choose a reason for hiding this comment

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

Not sure about these.

@bjorn3
Copy link
Member

bjorn3 commented Mar 30, 2022

I'm done reviewing. Apart from the remaining open review comments I think it there are no other mistakes here.

@RalfJung
Copy link
Member

RalfJung commented Mar 30, 2022

Given the amount of false positives here, we should maybe more explicitly ask for not just submitting tool-based spellcheck PRs? The reviewing load is not in good relation to the benefit. I recall @oli-obk and others having that discussion about punctuation PRs, not sure if any official policy was ever written based on that?

What would be really nice is a way to automatically fix common spelling mistakes and have that on CI so we know these issues won't come back in the future. But one-off "fixing a bunch of random spelling mistakes in various places in the codebase" (of which we had quite a few recently, based just on PRs that automatically ping me as a Miri team member) will always only help temporarily as new typos will be introduced soon.

@oli-obk
Copy link
Contributor

oli-obk commented Mar 30, 2022

We do have this policy. I was hoping the PRs were just one offs, but it is indeed getting too much again.

As per #58619 I am closing this PR and will close all upcoming ones, too.

If you do want to continue working on this, please consider developing tooling that can be applied without false positives like our formatting tooling.

@oli-obk oli-obk closed this Mar 30, 2022
@nyurik
Copy link
Contributor Author

nyurik commented Mar 30, 2022

Thanks @compiler-errors and @bjorn3 for the review! I will resolve all the simple issues you raised.

@RalfJung that's a fair point. I did add almost 2,000 words to my dictionary as valid, and those that slipped became false positives. That said -- I verified each one of them by hand, and I was fairly certain that the proposed changes are OK. My apologies for making mistakes.

@nyurik
Copy link
Contributor Author

nyurik commented Mar 30, 2022

@oli-obk would storing a list of words as part of this (or other) repo work? False positives only exist because of the natural dictionary limitations, so storing well known per-project/per-domain lists should fix it.

Per #58619 you posted -- Oh, and I haven't mentioned it, but it can't possibly annoy anyone – typo/spelling/grammatical fixes should be fine too. (per @alexreg) -- isn't that what i was following here?

One more thing -- would it make sense to not merge it after all the reviews were done? If there is a policy against spellchecking, I will be happy to not submit any more such PRs, but it seems this one is fairly innocuous and has already been vetted by two contributors.

@RalfJung
Copy link
Member

RalfJung commented Mar 30, 2022

No worries, mistakes happen. :) And I very much appreciate you taking the time to help improve Rust! ❤️

My concern is more generally with the cost-benefit trade-off for PRs like this. They touch a lot of code and hence automatically ping a lot of people and take sparse reviewer time away. And yet they only temporarily actually reduce the number of typos in the codebase. I'm not sure having a constant stream of such PRs (fixing the constant stream of new typos being added) is worth the constant load this puts on reviewers (reviewer time is a very sparse resource).

OTOH, being able to fix a particular spelling mistake once and for all (so ./x.py fmt takes care of it in the future) would be a very different situation. That's why @oli-obk and me keep asking for tools.

Note that alexreg was the person doing the style PRs back then, so this represents their view, not the view of the team(s).

@RalfJung
Copy link
Member

FWIW though I also agree that if @bjorn3 already spent the reviewer time and agrees the remaining changes are clearly fixing typos and there are no ambiguities, then there's no point in throwing away that work. 160 line diff isn't that bad re: risk of conflicts.

@oli-obk
Copy link
Contributor

oli-obk commented Mar 30, 2022

Yea, the work here has been done, so I'd be ok with merging it, but that's also what happend to the previous 5 PRs or so in the last two weeks.

I'm not going to block you or Bjorn3 moving ahead with this PR, but I'm going to block all future PRs until we get tooling.

@oli-obk
Copy link
Contributor

oli-obk commented Mar 30, 2022

would storing a list of words as part of this (or other) repo work? False positives only exist because of the natural dictionary limitations, so storing well known per-project/per-domain lists should fix it.

Yes, totally. As long as we can automatically apply it with ./x.py fmt and at some point gate CI on it, I'm for it.

The tooling must be easy to use on all platforms though, so there may be limitations to what tools can be used. Requiring contributors to install something is likely not going to get accepted. Python or Rust programs on the other hand will cause little friction

@compiler-errors
Copy link
Member

Given @bjorn3's and my reviews, and the comments above by @oli-obk and @RalfJung who are not opposed to this one landing, I'll reopen this.

I would also be quite elated to see a style linter in ./x.py fmt. I will also keep #58619 in mind if I see future style/formatting PRs in the review queue.

@compiler-errors
Copy link
Member

since bjorn3 said they were done reviewing too, and the only nits I see are minimal:
@bors r+

@bors
Copy link
Contributor

bors commented Mar 30, 2022

📌 Commit 8d7b124 has been approved by compiler-errors

@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 Mar 30, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 31, 2022
Rollup of 5 pull requests

Successful merges:

 - rust-lang#95130 (Stabilize thread::is_finished)
 - rust-lang#95263 (Restore `impl Future<Output = Type>` to async blocks)
 - rust-lang#95471 (Don't ICE when opaque types get their hidden type constrained again.)
 - rust-lang#95491 (Stabilize feature vec_retain_mut on Vec and VecDeque)
 - rust-lang#95497 (Spellchecking compiler comments)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 1b7d6db into rust-lang:master Mar 31, 2022
@rustbot rustbot added this to the 1.61.0 milestone Mar 31, 2022
@nyurik nyurik deleted the compiler-spell-comments branch July 23, 2022 01:00
@nyurik
Copy link
Contributor Author

nyurik commented Oct 18, 2023

@RalfJung I just found out about cargo-spellcheck - might be a good solution long term

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.

9 participants