Skip to content

Conversation

@mansellan
Copy link
Member

@mansellan mansellan commented Jun 23, 2018

Closes #4128

There were a few issues:

  1. Was using ASSIGN instead of EQ
  2. Was using lExpression instead of expression
  3. Was not referenced from mainBlockStmt
  4. No tests


[Category("Parser")]
[Test]
public void MidStatement()
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, we need negative tests, too to assert that an expression that contains a Mid() function does not get parsed as a mid statement. We also need equivalent tests for Mid$() statement/function.

For example, we definitely don't want something like If Mid$("foo", 1) = "f" Then to be treated as a Mid statement. The tests need to validate this.

@codecov
Copy link

codecov bot commented Jun 23, 2018

Codecov Report

Merging #4129 into next will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##             next    #4129      +/-   ##
==========================================
+ Coverage   52.55%   52.55%   +<.01%     
==========================================
  Files         963      963              
  Lines       33142    33140       -2     
==========================================
- Hits        17415    17414       -1     
+ Misses      15727    15726       -1
Impacted Files Coverage Δ
...rsing/Binding/Bindings/SimpleNameDefaultBinding.cs 85.25% <100%> (+0.38%) ⬆️
...ar/PartialExtensions/VBAParserPartialExtensions.cs 53.3% <0%> (ø) ⬆️

@bclothier
Copy link
Contributor

Also, make sure to check whether you can remove the "hack" from the resolver

//TODO - this is a complete and total hack to prevent `Mid` and `Mid$` from creating undeclared variables
//pending an actual fix to the grammar. See #2618
else if (!_name.Equals("Mid") && !_name.Equals("Mid$"))
{
var undeclaredLocal = _declarationFinder.OnUndeclaredVariable(_parent, _name, _context);
return new SimpleNameExpression(undeclaredLocal, ExpressionClassification.Variable, _context);
}
return new ResolutionFailedExpression();

@mansellan
Copy link
Member Author

mansellan commented Jun 23, 2018

image

Guessing that (function:Variant) is due to function aliasing. But it's not resolving it to a variable now :-)

@bclothier
Copy link
Contributor

I wonder if it's necessary to have additional unit tests to prove that it's not an undeclared variable to directly test the effect of the hack that was removed?

Copy link
Member

@retailcoder retailcoder left a comment

Choose a reason for hiding this comment

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

Approving because resolving as a function is a huge step forward compared to resolving to an undeclared variable (i.e. not resolving it at all), however Mid$ needs to resolve to a String-returning function; the inspection for string-typed functions will be having false negatives until that's fixed.

@retailcoder retailcoder merged commit fa78fb4 into rubberduck-vba:next Jun 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants