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

Minor improvements to rustc_ast_passes #117204

Merged
merged 7 commits into from
Nov 2, 2023

Conversation

nnethercote
Copy link
Contributor

Some improvements I found while looking at this code.

r? @compiler-errors

@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 Oct 26, 2023
@rustbot
Copy link
Collaborator

rustbot commented Oct 26, 2023

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

crate::fluent_generated::ast_passes_forbidden_non_lifetime_param,
)
.emit();
if !non_lt_param_spans.is_empty() {
Copy link
Member

Choose a reason for hiding this comment

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

What this will do is emit a feature error for every single span. What I want is for it to be a single feature error with multiple primary spans.

I prefer if this is not changed, or it is fixed in the right way.

Copy link
Member

Choose a reason for hiding this comment

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

IDK why there isn't a test for this. Here's one:

fn test() -> for<T, S> fn(T, S) { todo!() }
error[E0658]: only lifetime parameters can be used in this context
 --> src/lib.rs:1:18
  |
1 | fn test() -> for<T, S> fn(T, S) { todo!() }
  |                  ^  ^
  |
  = note: see issue #108185 <https://github.com/rust-lang/rust/issues/108185> for more information

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

I have updated the code, removing the invalid commit, and adding three new commits: another small refactor, an alternative fix for the FIXME, and a test update. Hopefully that's closer to what is necessary :) Those macros still annoy me, but I can't see a better way to do things without increasing the amount of boilerplate.

The debug probably isn't useful, and assigning all the `$foo`
metavariables to `foo` variables is verbose and weird. Also, `$x:expr`
usually doesn't have a space after the `:`.
Note that this adds the `span.allows_unstable` checking that this case
previously lacked.
@nnethercote
Copy link
Contributor Author

I fixed the conflicts.

@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Nov 2, 2023

📌 Commit 5b391b0 has been approved by compiler-errors

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 Nov 2, 2023
@bors
Copy link
Contributor

bors commented Nov 2, 2023

⌛ Testing commit 5b391b0 with merge 62270fb...

@bors
Copy link
Contributor

bors commented Nov 2, 2023

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 62270fb to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 2, 2023
@bors bors merged commit 62270fb into rust-lang:master Nov 2, 2023
12 checks passed
@rustbot rustbot added this to the 1.75.0 milestone Nov 2, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (62270fb): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
3.1% [3.0%, 3.2%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 639.997s -> 639.068s (-0.15%)
Artifact size: 304.49 MiB -> 304.47 MiB (-0.01%)

bors-ferrocene bot added a commit to ferrocene/ferrocene that referenced this pull request Nov 3, 2023
82: Automated pull from upstream `master` r=tshepang a=github-actions[bot]


This PR pulls the following changes from the upstream repository:

* rust-lang/rust#117313
* rust-lang/rust#117131
* rust-lang/rust#117134
* rust-lang/rust#117471
* rust-lang/rust#117521
* rust-lang/rust#117513
  * rust-lang/rust#117512
  * rust-lang/rust#117509
  * rust-lang/rust#117495
  * rust-lang/rust#117394
* rust-lang/rust#117466
* rust-lang/rust#117204
* rust-lang/rust#117386
* rust-lang/rust#117506



Co-authored-by: Nicholas Nethercote <n.nethercote@gmail.com>
Co-authored-by: roblabla <unfiltered@roblab.la>
Co-authored-by: Michael Goulet <michael@errs.io>
Co-authored-by: massivebird <gdrakemail@gmail.com>
Co-authored-by: bors <bors@rust-lang.org>
Co-authored-by: Zalathar <Zalathar@users.noreply.github.com>
Co-authored-by: lcnr <rust@lcnr.de>
Co-authored-by: Joshua Liebow-Feeser <joshlf@users.noreply.github.com>
Co-authored-by: Matthias Krüger <matthias.krueger@famsik.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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.

5 participants