There is no variance to infer for generics that occur in non-variance contexts #13261
Conversation
|
The more I reflect on sha 8599236 , the more I think I should explore the alternative approach I outlined in the commit message (i.e., the |
| kind: ParamKind, | ||
| index: uint, | ||
| param_id: ast::NodeId) | ||
| { |
There was a problem hiding this comment.
Nit: not that I care, but I think our official coding standards moves such braces to the previous line.
|
Ultimately I agree with everything you wrote @pnkfelix. This seems like a good sanity check. It might be better to only enable it if assertions are enabled. It might be better still to just have a way to go from the def-id of a region parameter (in this case, the early bound parameter) to the item that declared it, which is really all you're trying to infer here. I'm not actually sure if such a thing exists, the usual thing would be to add it to the ast-map I guess. |
|
feedback? @eddyb (especially regarding commit dc37ba3 ) |
|
LGTM logic-wise, at least. |
|
When did we gain an invariant about when the "new_id" is called? Was it always like that and I didn't realize? |
|
@pnkfelix this looks pretty good. My inclination would be to have the canonical path be checking the inferred map, and have the ast path you wrote out be an assertion, but I'm really just micro-optimizing here. |
|
(r+ once merge conflicts are fixed -- but considering making the inferred map be the normal path) |
Before adding a variance constrant for a given early-bound param, check if it was meant to be inferred. To support the above, added `fn is_to_be_inferred` to `variance::ConstraintContext`.
|
@nikomatsakis I believe the invariant about when |
Part of this required added an override of `fold_type_method` in the
Folder for Ctx impl; it follows the same pattern as `fold_method`.
Also, as a drive-by fix, I moved all of the calls to `folder.new_id`
in syntax::fold's no-op default traversal to really be the first
statement in each function.
* This is to uphold the invariant that `folder.new_id` is always
called first (an unfortunate requirement of the current `ast_map`
code), an invariant that we seemingly were breaking in e.g. the
previous `noop_fold_block`.
* Now it should be easier to see when adding new code that this
invariant must be upheld.
* (note that the breakage in `noop_fold_block` may not have mattered
so much previously, since the only thing that blocks can bind are
lifetimes, which I am only adding support for now.)
This version of `is_to_be_inferred` double-checks the result from `inferred_map` by querying the `named_region_map` and `ast_map` and then asserts that the `inferred_map` state is consistent with its own findings. (See issue 13261 for further discussion of the approaches).
|
(I'm going to treat the question from @nikomatsakis about when the invariant was introduced as evidence that I am correct to attempt to make the invariant more apparent in the code.) |
pnkfelix
changed the title
pnkfelix
changed the title
Fix #12856. I wanted to put this up first because I wanted to get feedback about the second commit in the series, commit 8599236. Its the more invasive part of the patch and is largely just belt-and-suspenders assertion checking; in the commit message I mentioned at least one other approach we could take here. Or we could drop the belt-and-suspenders and just rely on the guard added in the first patch, commit 8d6a005 (which is really quite trivial on its own). So any feedback on what would be better is appreciated. r? @nikomatsakis
Fix #12856.
I wanted to put this up first because I wanted to get feedback about the second commit in the series, commit 8599236. Its the more invasive part of the patch and is largely just belt-and-suspenders assertion checking; in the commit message I mentioned at least one other approach we could take here. Or we could drop the belt-and-suspenders and just rely on the guard added in the first patch, commit 8d6a005 (which is really quite trivial on its own).
So any feedback on what would be better is appreciated.
r? @nikomatsakis