Repository navigation
Move hir::Params from Body of functions to FnDecl - #163758
JonathanBrouwer wants to merge 13 commits into
Conversation
rustbot
added
S-waiting-on-author
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
💔 Test for be17f02 failed: CI. Failed jobs:
|
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (b0e36da): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. Next, please: If you can, justify the regressions found in this try perf run in writing along with @bors rollup=never rustc-perf Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (secondary 0.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 3.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 490.122s -> 491.234s (0.23%) |
rustbot
added
perf-regression
This comment has been minimized.
This comment has been minimized.
10e9505 to
67a82db
Compare
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
1ef0e8b to
6d88206
Compare
This commit is by far the biggest one, and achieves the goal of this PR but leaves the code ugly in a lot of places. The next commits will be a lot of cleanup.
6d88206 to
1185465
Compare
|
|
||
| /// A `hir::Param` without a `ty`, which exists after the `Param` is lowered, before the corresponding `ty` is lowered. | ||
| #[derive(Clone, Debug)] | ||
| pub(crate) struct PartialParam<'hir> { |
There was a problem hiding this comment.
This PR introduces the PartialParam attempt in order to keep the control flow in this crate as much as possible the same, in an aim to keep things reviewable.
It will take some more refactoring to disentangle lowering Body with lowering the Param, mostly because of closures.
I'm aiming to do this in a follow-up PR, since this PR is already big enough.
| /// This variant is not always used to represent inference types, sometimes | ||
| /// [`GenericArg::Infer`] is used instead. | ||
| Infer(Unambig), | ||
| /// Only used as the type of C-variadic (...) parameters |
There was a problem hiding this comment.
This PR introduces the hir::TyKind::CVarArgs, this corresponds to ast::TyKind::CVargs.
Previously CVarArgs parameters were represented by a hir::Param, but NOT by a corresponding entry in FnDecl::inputs, causing the lists to be differing lengths. This is confusing and this PR fixes this.
| } | ||
|
|
||
| fn visit_body(&mut self, b: &rustc_hir::Body<'tcx>) -> Self::Result { | ||
| if let Some(decl) = self.tcx.hir_fn_decl_by_hir_id(self.tcx.hir_body_owner(b.id())) { |
There was a problem hiding this comment.
A few visitors need to be adjusted to now also visit the fn_decl.
I couldn't really find a cleaner solution than this
| let input_tys = decl | ||
| .inputs | ||
| .iter() | ||
| .filter(|p| !matches!(p.ty.kind, hir::TyKind::CVarArgs)) |
There was a problem hiding this comment.
This filter will happen in a few places, previously CVarArgs were inherently ignored because they did not have an entry in decl.inputs, now they do (more consistent0 and we explicitly filter them out to keep the logic the same
| fn check_pat(&mut self, cx: &LateContext<'_>, p: &hir::Pat<'_>) { | ||
| // Ignore patterns in foreign items for now | ||
| // FIXME(#33995) remove this check | ||
| if let Node::ForeignItem(_) = cx |
There was a problem hiding this comment.
Removing this if would fix a bug, but this is a behaviour change so I would like to do that in a follow-up PR
#33995
| if let Some(hir_id) = hir_id | ||
| && let hir::Node::Param(param) = self.tcx.hir_node(hir_id) | ||
| && let Some(decl) = self.tcx.parent_hir_node(hir_id).fn_decl() | ||
| && let Some(t) = decl.inputs.iter().find(|t| param.ty_span.contains(t.span)) |
There was a problem hiding this comment.
This is a weird hack to find the param ty from a param, we no longer need to do this :)
| impl Bar { | ||
| //@ has assoc_consts/struct.Bar.html '//*[@id="associatedconstant.F"]' \ | ||
| // "const F: fn(&(dyn ToString + 'static))" | ||
| // "const F: fn(_: &(dyn ToString + 'static))" |
There was a problem hiding this comment.
Previously rustdoc always filtered out wildcards, after this PR it matches what the user wrote doen, which I think is prettier (and makes the code easier :p)
| | | ||
| LL | fn dstfn(src: i32, dst: err); | ||
| | ^^^^^ --- | ||
| | ^^^^^ -------- |
There was a problem hiding this comment.
A lot of spans changed to show the entire parameter (incl ty) rather than just the name, I think this is prettier (and again, easier to program :p)
| hir-stats ForeignItemId 4 (NN.N%) 1 4 | ||
| hir-stats ---------------------------------------------------------------- | ||
| hir-stats Total 8_592 172 | ||
| hir-stats Total 8_768 176 |
There was a problem hiding this comment.
This got bigger because we now allocate Param for functions without bodies. This is probably also related to the small perf regression
The field is now a duplicate of `Param::ty`'s span
This makes the span of parameters point to the entire parameter, for functions without bodies. This matches the behaviour of functions with bodies.
This improves performance :)
1185465 to
bbb9327
Compare
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt HIR ty lowering was modified cc @fmease Changes to the size of AST and/or HIR nodes. cc @nnethercote Some changes occurred in compiler/rustc_passes/src/check_attr.rs Some changes occurred in need_type_info.rs cc @lcnr
cc @rust-lang/clippy |
|
@ada4a Would you check the clippy diff for me? |
|
peaceful friday evening me rn: 
jokes aside, just give me few more days, i'll review this eventually |
I've been low on energy lately due to the move.. I'll try to get to this during this/next weekend^^ |
|
What this blocked on (asking because of s-blocked label) |
|
This was blocked on the MCP being accepted, it now is :) |
|
@nnethercote would you like to take a look at this PR? |
There was a problem hiding this comment.
Diagnostics for this code will change, and not in a better way
struct NeedsCopy<T: Copy>(T);
extern "C" {
fn f(x: i32, ...) -> NeedsCopy<String>;
}Previously it gave this:
error[E0277]: the trait bound `String: Copy` is not satisfied
--> test.rs:3:26
|
3 | fn f(x: i32, ...) -> NeedsCopy<String>;
| ^^^^^^^^^^^^^^^^^ the trait `Copy` is not implemented for `String`
now it gives this:
error[E0277]: the trait bound `String: Copy` is not satisfied
--> test.rs:3:18
|
3 | fn f(x: i32, ...) -> NeedsCopy<String>;
| ^^^ the trait `Copy` is not implemented for `String`
|
Fixed it + added a regression test. |
| } | ||
|
|
||
| #[derive(Debug, Copy, Clone)] | ||
| enum DiscardParams { |
There was a problem hiding this comment.
It was deleted but there is a comment still referencing to it
rust/compiler/rustc_ast_lowering/src/lib.rs
Line 1931 in 9358362
| } | ||
|
|
||
| impl<'tcx> Visitor<'tcx> for NamePrivacyVisitor<'tcx> { | ||
| fn visit_fn_decl(&mut self, _fd: &'tcx FnDecl<'tcx>) -> Self::Result {} |
There was a problem hiding this comment.
I see what are you trying to do here, but it feel like you forget to update visit_nested_body to walk the pattern params, so currently it breaks (another thing that doesn't covers in tests, fun)
mod hidden {
pub struct Record { secret: u8 }
}
fn leak(hidden::Record { secret }: hidden::Record) -> u8 {
secret
}This code is currently correctly rejects on main, but compiles under this PR
There was a problem hiding this comment.
Also, please when you are going fix this, make sure this also rejects correctly
mod hidden {
pub struct Record { secret: u8 }
}
fn f() -> [u8; { let _ = hidden::Record { secret: 1 }; 0 }] {
[]
}
| cannot_remove: FxHashSet::default(), | ||
| desugar_binding_spans: Vec::new(), | ||
| }; | ||
| finder.visit_fn_decl(tcx.hir_fn_decl(self.mir_hir_id())); |
There was a problem hiding this comment.
This will 🧊
struct NonCopy(u8);
const C: () = {
let &moved = &NonCopy(0);
};
View all comments
This PR implements MCP rust-lang/compiler-team#1044.
It is split up into commits, each commit passes tests, and reviewing commit by commit is therefore heavily recommended. Each commit contains a description. The first commit makes the big change, and the other commits are cleanup.
This PR causes a minor performance regression, specifically on the
libcandexternsbenchmark:#163758 (comment)
This is caused by that parameters on foreign functions are now assigned HIR IDs and
Params are allocated for them, so slightly more work is needed.Both of these crates are basically only foreign functions, so not very representative of an average rust program.
r? @Kivooeo