fix: Do not infer signatures, instead infer anon consts in them#22198
Conversation
| 223..227 'iter': IntoIter<u8> | ||
| 230..231 'a': Vec<u8> | ||
| 230..243 'a.into_iter()': IntoIter<u8> | ||
| 322..323 '1': usize |
There was a problem hiding this comment.
Previously we inferred all expressions in the signature. Now we skip literals and paths (we don't create an anon const for them) to save time and space. The net effect is that they aren't shown in tests and to IDE, although it's possibly to support them for IDE in little effort.
There was a problem hiding this comment.
How big are the savings here? Imo we should definitely make these work for IDE analysis
| ); | ||
| // note: this may break later if we add more consteval. it just needs to be something that our | ||
| // consteval engine doesn't understand | ||
| check_assist_not_applicable( |
There was a problem hiding this comment.
It's not easy to support this now, since the type does not contain an error type anymore: it just has an anon const that fails to evaluate, which causes it to fail to normalize. But I've realized this test is no longer needed, since _ in arrays is supported by now.
047f10d to
372b99f
Compare
This comment has been minimized.
This comment has been minimized.
372b99f to
f74e82e
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
f74e82e to
b5b1afd
Compare
This comment has been minimized.
This comment has been minimized.
b5b1afd to
d9151a7
Compare
This comment has been minimized.
This comment has been minimized.
d9151a7 to
e6823d8
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
e6823d8 to
c955672
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
c955672 to
b7a0d1e
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
b7a0d1e to
64135cf
Compare
This comment has been minimized.
This comment has been minimized.
64135cf to
6b8e45d
Compare
This comment has been minimized.
This comment has been minimized.
6b8e45d to
8e45e71
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
| let konst = create_anon_const( | ||
| self.interner, | ||
| self.def, | ||
| self.store, | ||
| const_ref.expr, | ||
| self.resolver, | ||
| const_type, | ||
| &|| self.generics(), | ||
| None, | ||
| self.forbid_params_after, | ||
| ); |
There was a problem hiding this comment.
In the Expr::Underscore case, because this doesn't provide infcx here, create_anon_const returns an Err which will reach the unwrap_or case below. I believe we want to create a new const var instead?
(I tripped on this when rebasing my related PR on top of this.)
There was a problem hiding this comment.
We don't have access to the InferCtxt here, only after #22237 we will.
| // The repeat is an anon const. | ||
| &hir_def::hir::Expr::Array(hir_def::hir::Array::Repeat { | ||
| initializer, | ||
| repeat: _, | ||
| }) => append_coroutines_in_expr(db, owner, store, initializer, result), | ||
| _ => store.walk_child_exprs(expr_id, |expr_id| { | ||
| append_coroutines_in_expr(db, owner, store, expr_id, result) | ||
| }), | ||
| } |
There was a problem hiding this comment.
This sounds like we should have a walk_child_exprs version that exhaustively matches and skips anon const places. Otherwise it is easy for us to miss this if a new case arises
There was a problem hiding this comment.
I want to change the visitors to be based on traits anyway (have a branch for that), that'll be a good place to do this.
| 223..227 'iter': IntoIter<u8> | ||
| 230..231 'a': Vec<u8> | ||
| 230..243 'a.into_iter()': IntoIter<u8> | ||
| 322..323 '1': usize |
There was a problem hiding this comment.
How big are the savings here? Imo we should definitely make these work for IDE analysis
| /// An anonymous const expression that appears in a type position (e.g., array lengths, | ||
| /// const generic arguments like `{ N + 1 }`). Unlike named constants, these don't have |
There was a problem hiding this comment.
Aren't const param defaults also anon consts as well as field defaults?
There was a problem hiding this comment.
Const param defaults, yes. Field defaults, I don't think we lower those yet.
There was a problem hiding this comment.
We are given the IDE layer now supports it
rust-analyzer/crates/hir/src/source_analyzer.rs
Lines 89 to 94 in 1cab8f7
8e45e71 to
40afaeb
Compare
See [the Zulip discussion](https://rust-lang.zulipchat.com/#narrow/channel/185405-t-compiler.2Frust-analyzer/topic/Anon.20consts.20for.20everything/with/587684630) for details, but in short: - This allows us to easily set the expected type. - They can have proper MIR an eval, preventing the need for hacks like `fixme_resolve_all_clone()`. This also fixes a bunch of old inference failures caused by us unable to evaluate constants. In order to make them visible to the IDE layer, each query that can create anon consts now keeps a list of the anon consts it created. The expression store also gains an ability to find the root expression of something. Then, in `Semantics`, when wanting to find inference info for something, we find its root expr, then search it in the list of anon consts defined by the body.
40afaeb to
2bbdac0
Compare
They are big, 117mb on self. But as I said we can still support them in IDE, I'll work on that in a follow-up PR. |
See the Zulip discussion for details, but in short:
fixme_resolve_all_clone().This also fixes a bunch of old inference failures caused by us unable to evaluate constants.
In order to make them visible to the IDE layer, each query that can create anon consts now keeps a list of the anon consts it created. The expression store also gains an ability to find the root expression of something. Then, in
Semantics, when wanting to find inference info for something, we find its root expr, then search it in the list of anon consts defined by the body.This should reduce our diagnostics on self by at least two (they're caused by failure to evaluate consts on
to_le_bytes(), we even have a test for that).Blocked on #22194.