Make hover tests stricter, add type narrowing test, fix up proc hover. #902
Conversation
(Proc hover tests were passing before w/o the strict checks, but should have failed.)
| @@ -334,7 +334,7 @@ BasicBlock *CFGBuilder::walk(CFGContext cctx, ast::Expression *what, BasicBlock | |||
| InlinedVector<core::LocalVariable, 2> idxVec{idxTmp}; | |||
| InlinedVector<core::Loc, 2> locs{zeroLengthLoc}; | |||
| bodyBlock->exprs.emplace_back( | |||
| argLoc, zeroLengthLoc, | |||
| argLoc, arg.loc, | |||
There was a problem hiding this comment.
When I updated this code before, I thought that the loc query for arg.loc would get argLoc -- the local variable representing the argument. However, it does not because no cfg::Ident expression is added to the method.
As a hack, I've changed this Send binding (which is args[i]) to use arg.loc so it gets picked up instead. The hover for proc argument is now sig {returns(type-of-arg)}, which is more correct than before but not ideal.
There was a problem hiding this comment.
If I can figure out that a cfg::Send has a block argument, and if I can get the types+locs of its parameter declarations, I could possibly do this the right way and match against the parameter in Environment::processBindings.
Is that possible in cfg?
There was a problem hiding this comment.
@jvilk-stripe I think you're looking for LoadYieldArg instruction
There was a problem hiding this comment.
Please, feel free to either land it as is or follow your ideas in #902 (comment)
|
Going to land as-is and follow up with improvements. |
Make hover tests stricter, add a type narrowing test, fix up proc hover.
This change makes hover tests match against whole lines and not just a substring of the entire hover contents.
Motivation
Without this change, I can't assert that something is
Stringand notT.nilable(String).This change also surfaced a bug in hover on proc arguments. We had a test that was passing when it should have been failing.
Test plan
See included automated tests.