Skip to content

Commit 3fe4074

Browse files
retailcoderretailcoder
authored
Merge pull request #3891 from BZngr/3868_Comments
addresses review comments, ref. #3868 / unreachable case inspection
2 parents 77ff013 + 8ac381c commit 3fe4074

File tree

6 files changed

+235
-59
lines changed

6 files changed

+235
-59
lines changed

‎Rubberduck.Inspections/Concrete/UnreachableCaseInspection/ParseTreeExpressionEvaluator.cs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -153,10 +153,11 @@ internal static class MathSymbols
153153
private static string _plus;
154154
private static string _minusSign;
155155
private static string _exponent;
156+
private static string _integerDivide;
156157

157158
public static string MULTIPLY => _multiply ?? LoadSymbols(VBAParser.MULT);
158159
public static string DIVIDE => _divide ?? LoadSymbols(VBAParser.DIV);
159-
public static string INTEGER_DIVIDE => @"\";
160+
public static string INTEGER_DIVIDE => _integerDivide ?? LoadSymbols(VBAParser.INTDIV);
160161
public static string PLUS => _plus ?? LoadSymbols(VBAParser.PLUS);
161162
public static string MINUS => _minusSign ?? LoadSymbols(VBAParser.MINUS);
162163
public static string ADDITIVE_INVERSE => MINUS;
@@ -167,6 +168,7 @@ private static string LoadSymbols(int target)
167168
{
168169
_multiply = VBAParser.DefaultVocabulary.GetLiteralName(VBAParser.MULT).Replace("'", "");
169170
_divide = VBAParser.DefaultVocabulary.GetLiteralName(VBAParser.DIV).Replace("'", "");
171+
_integerDivide = VBAParser.DefaultVocabulary.GetLiteralName(VBAParser.INTDIV).Replace("'", "");
170172
_plus = VBAParser.DefaultVocabulary.GetLiteralName(VBAParser.PLUS).Replace("'", "");
171173
_minusSign = VBAParser.DefaultVocabulary.GetLiteralName(VBAParser.MINUS).Replace("'", "");
172174
_exponent = VBAParser.DefaultVocabulary.GetLiteralName(VBAParser.POW).Replace("'", "");

‎Rubberduck.Inspections/Concrete/UnreachableCaseInspection/ParseTreeValueVisitor.cs

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -241,7 +241,6 @@ private bool TryGetLExprValue(VBAParser.LExprContext lExprContext, out string ex
241241
{
242242
expressionValue = string.Empty;
243243
declaredTypeName = string.Empty;
244-
245244
if (lExprContext.TryGetChildContext(out VBAParser.MemberAccessExprContext memberAccess))
246245
{
247246
var member = memberAccess.GetChild<VBAParser.UnrestrictedIdentifierContext>();
@@ -280,9 +279,9 @@ private void GetContextValue(ParserRuleContext context, out string declaredTypeN
280279
private bool TryGetIdentifierReferenceForContext(ParserRuleContext context, out IdentifierReference idRef)
281280
{
282281
idRef = null;
283-
var nameToMatch = context.GetText();
284-
var identifierReferences = (_state.DeclarationFinder.MatchName(context.GetText()).Select(dec => dec.References)).SelectMany(rf => rf);
285-
if (identifierReferences.Any(rf => rf.Context == context))
282+
var identifierReferences = (_state.DeclarationFinder.MatchName(context.GetText()).Select(dec => dec.References)).SelectMany(rf => rf)
283+
.Where(rf => rf.Context == context);
284+
if (identifierReferences.Count() == 1)
286285
{
287286
idRef = identifierReferences.First();
288287
return true;
@@ -320,11 +319,13 @@ private string GetBaseTypeForDeclaration(Declaration declaration)
320319
{
321320
var localDeclaration = declaration;
322321
var iterationGuard = 0;
323-
while (!localDeclaration.AsTypeIsBaseType && iterationGuard++ < 5)
322+
while (!(localDeclaration is null)
323+
&& !localDeclaration.AsTypeIsBaseType
324+
&& iterationGuard++ < 5)
324325
{
325326
localDeclaration = localDeclaration.AsTypeDeclaration;
326327
}
327-
return localDeclaration.AsTypeName;
328+
return localDeclaration is null ? declaration.AsTypeName : localDeclaration.AsTypeName;
328329
}
329330

330331
private static bool IsBinaryMathContext<T>(T context)

‎Rubberduck.Inspections/Concrete/UnreachableCaseInspection/RangeClauseFilter.cs

Lines changed: 35 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,8 @@ public class RangeClauseFilter<T> : IRangeClauseFilter, IRangeClauseFilterTestSu
4444
private bool _hasExtents;
4545
private T _minExtent;
4646
private T _maxExtent;
47+
private string _cachedDescriptor;
48+
private bool _descriptorIsDirty;
4749

4850
public RangeClauseFilter(string typeName, IParseTreeValueFactory valueFactory, IRangeClauseFilterFactory filterFactory, TryConvertParseTreeValue<T> tConverter)
4951
{
@@ -61,6 +63,8 @@ public RangeClauseFilter(string typeName, IParseTreeValueFactory valueFactory, I
6163
_falseValue = ConvertToContainedGeneric(false);
6264
_trueValue = ConvertToContainedGeneric(true);
6365
TypeName = typeName;
66+
_cachedDescriptor = string.Empty;
67+
_descriptorIsDirty = true;
6468
}
6569

6670
private List<Tuple<T, T>> RangeValues => _ranges;
@@ -83,6 +87,10 @@ public bool FiltersAllValues
8387
{
8488
get
8589
{
90+
if (ContainsBooleans && CoversTrueFalse())
91+
{
92+
return true;
93+
}
8694

8795
var coversAll = false;
8896
var hasLTFilter = TryGetIsLTValue(out T ltValue);
@@ -101,10 +109,6 @@ public bool FiltersAllValues
101109
coversAll = gt - lt + 1 <= RangesValuesCount() + SingleValues.Count()
102110
|| gt == lt && RangesFilterValue(ltValue);
103111
}
104-
else if (ContainsBooleans && !coversAll)
105-
{
106-
coversAll = SingleValues.Contains(_trueValue);
107-
}
108112
return coversAll;
109113
}
110114
}
@@ -115,9 +119,9 @@ public bool ContainsFilters
115119
{
116120
return _ranges.Any() || _variableRanges.Any()
117121
|| _singleValues.Any() || _variableSingles.Any()
122+
|| _relationalOps.Any()
118123
|| TryGetIsLTValue(out T isLT) && isLT.CompareTo(_minExtent) != 0
119-
|| TryGetIsGTValue(out T isGT) && isGT.CompareTo(_maxExtent) != 0
120-
|| _relationalOps.Any();
124+
|| TryGetIsGTValue(out T isGT) && isGT.CompareTo(_maxExtent) != 0;
121125
}
122126
}
123127

@@ -152,6 +156,12 @@ public void Add(IRangeClauseFilter filter)
152156
{
153157
AddSingleValueImpl(val);
154158
}
159+
160+
foreach (var val in newFilter.VariableSingleValues)
161+
{
162+
VariableSingleValues.Add(val);
163+
}
164+
_descriptorIsDirty = true;
155165
}
156166

157167
public void AddIsClause(IParseTreeValue value, string opSymbol)
@@ -168,6 +178,7 @@ public void AddIsClause(IParseTreeValue value, string opSymbol)
168178
{
169179
AddRelationalOpImpl(value.ValueText);
170180
}
181+
_descriptorIsDirty = true;
171182
}
172183

173184
public void AddRelationalOp(IParseTreeValue value)
@@ -184,6 +195,7 @@ public void AddRelationalOp(IParseTreeValue value)
184195
{
185196
AddRelationalOpImpl(value.ValueText);
186197
}
198+
_descriptorIsDirty = true;
187199
}
188200

189201
public void AddSingleValue(IParseTreeValue value)
@@ -200,6 +212,7 @@ public void AddSingleValue(IParseTreeValue value)
200212
{
201213
VariableSingleValues.Add(value.ValueText);
202214
}
215+
_descriptorIsDirty = true;
203216
}
204217

205218
public void AddValueRange(IParseTreeValue inputStartVal, IParseTreeValue inputEndVal)
@@ -225,6 +238,7 @@ public void AddValueRange(IParseTreeValue inputStartVal, IParseTreeValue inputEn
225238
{
226239
AddVariableRangeImpl(inputStartVal.ValueText, inputEndVal.ValueText);
227240
}
241+
_descriptorIsDirty = true;
228242
}
229243

230244
public IRangeClauseFilter FilterUnreachableClauses(IRangeClauseFilter filter)
@@ -309,6 +323,11 @@ public override bool Equals(object obj)
309323

310324
public override string ToString()
311325
{
326+
if (!_descriptorIsDirty)
327+
{
328+
return _cachedDescriptor;
329+
}
330+
312331
var descriptors = new HashSet<string>
313332
{
314333
GetIsClausesDescriptor(LogicSymbols.LT),
@@ -329,7 +348,9 @@ public override string ToString()
329348
}
330349
descriptor.Append(descriptors.ElementAt(idx));
331350
}
332-
return descriptor.ToString();
351+
_cachedDescriptor = descriptor.ToString();
352+
_descriptorIsDirty = false;
353+
return _cachedDescriptor;
333354
}
334355

335356
public override int GetHashCode()
@@ -357,8 +378,7 @@ private bool FiltersAllRelationalOps
357378
{
358379
if (ContainsBooleans)
359380
{
360-
return _singleValues.Contains(_trueValue) && _singleValues.Contains(_falseValue)
361-
|| RangesFilterValue(_trueValue) && RangesFilterValue(_falseValue);
381+
return CoversTrueFalse();
362382
}
363383
return _singleValues.Contains(_trueValue) && _singleValues.Contains(_falseValue)
364384
|| RangesFilterValue(_trueValue) && RangesFilterValue(_falseValue)
@@ -367,6 +387,12 @@ private bool FiltersAllRelationalOps
367387
}
368388
}
369389

390+
private bool CoversTrueFalse()
391+
{
392+
return _singleValues.Contains(_trueValue) && _singleValues.Contains(_falseValue)
393+
|| RangesFilterValue(_trueValue) && RangesFilterValue(_falseValue);
394+
}
395+
370396
private void RemoveIsLTClause() => RemoveIsClauseImpl(LogicSymbols.LT);
371397

372398
private void RemoveIsGTClause() => RemoveIsClauseImpl(LogicSymbols.GT);

‎Rubberduck.Inspections/Concrete/UnreachableCaseInspection/SelectCaseStmtContextWrapper.cs

Lines changed: 31 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@ public class SelectCaseStmtContextWrapper : ContextWrapperBase, ISelectCaseStmtC
2222
private List<ParserRuleContext> _unreachableResults;
2323
private List<ParserRuleContext> _mismatchResults;
2424
private List<ParserRuleContext> _caseElseResults;
25-
private string _evalTypeName;
2625

2726
public SelectCaseStmtContextWrapper(VBAParser.SelectCaseStmtContext selectCaseContext, IParseTreeVisitorResults inspValues, IUnreachableCaseInspectionFactoryProvider factoryFactory)
2827
: base(selectCaseContext, inspValues, factoryFactory)
@@ -31,12 +30,11 @@ public SelectCaseStmtContextWrapper(VBAParser.SelectCaseStmtContext selectCaseCo
3130
_unreachableResults = new List<ParserRuleContext>();
3231
_mismatchResults = new List<ParserRuleContext>();
3332
_caseElseResults = new List<ParserRuleContext>();
34-
_evalTypeName = null;
3533
_inspectionRangeFactory = factoryFactory.CreateIRangeClauseContextWrapperFactory();
34+
SetEvaluationTypeName(Context, ParseTreeValueResults, FilterFactory);
3635
}
3736

38-
public string EvaluationTypeName => _evalTypeName
39-
?? DetermineSelectCaseEvaluationTypeName(Context, ParseTreeValueResults, FilterFactory);
37+
public string EvaluationTypeName { private set; get; }
4038

4139
public void InspectForUnreachableCases()
4240
{
@@ -80,28 +78,43 @@ public void InspectForUnreachableCases()
8078

8179
public List<ParserRuleContext> UnreachableCaseElseCases => _caseElseResults;
8280

83-
private static bool InspectionCanEvaluateTypeName(string typeName) => !(typeName == string.Empty || typeName == Tokens.Variant);
84-
85-
private string DetermineSelectCaseEvaluationTypeName(ParserRuleContext context, IParseTreeVisitorResults inspValues, IRangeClauseFilterFactory factory)
81+
private static List<string> InspectableTypes = new List<string>()
82+
{
83+
Tokens.Byte,
84+
Tokens.Integer,
85+
Tokens.Int,
86+
Tokens.Long,
87+
Tokens.LongLong,
88+
Tokens.Single,
89+
Tokens.Double,
90+
Tokens.Decimal,
91+
Tokens.Currency,
92+
Tokens.Boolean,
93+
Tokens.String
94+
};
95+
96+
private static bool InspectionCanEvaluateTypeName(string typeName) => InspectableTypes.Contains(typeName);
97+
98+
private void SetEvaluationTypeName(ParserRuleContext context, IParseTreeVisitorResults inspValues, IRangeClauseFilterFactory factory)
8699
{
87100
var selectStmt = (VBAParser.SelectCaseStmtContext)context;
88-
if (TryDetectTypeHint(selectStmt.selectExpression().GetText(), out _evalTypeName))
101+
if (TryDetectTypeHint(selectStmt.selectExpression().GetText(), out string evalTypName))
89102
{
90-
return _evalTypeName;
103+
EvaluationTypeName = evalTypName;
104+
return;
91105
}
92106

93107
var typeName = string.Empty;
94108
if (inspValues.TryGetValue(selectStmt.selectExpression(), out IParseTreeValue result))
95109
{
96-
_evalTypeName = result.TypeName;
110+
EvaluationTypeName = result.TypeName;
97111
}
98112

99-
if (InspectionCanEvaluateTypeName(_evalTypeName))
113+
if (InspectionCanEvaluateTypeName(EvaluationTypeName))
100114
{
101-
return _evalTypeName;
115+
return;
102116
}
103-
_evalTypeName = DeriveTypeFromCaseClauses(inspValues, selectStmt);
104-
return _evalTypeName;
117+
EvaluationTypeName = DeriveTypeFromCaseClauses(inspValues, selectStmt);
105118
}
106119

107120
private string DeriveTypeFromCaseClauses(IParseTreeVisitorResults inspValues, VBAParser.SelectCaseStmtContext selectStmt)
@@ -118,21 +131,16 @@ private string DeriveTypeFromCaseClauses(IParseTreeVisitorResults inspValues, VB
118131
else
119132
{
120133
var inspRange = _inspectionRangeFactory.Create(range, inspValues);
121-
var types = inspRange.ResultContexts.Select(rc => inspValues.GetTypeName(rc));
134+
var types = inspRange.ResultContexts.Select(rc => inspValues.GetTypeName(rc))
135+
.Where(tp => InspectableTypes.Contains(tp));
122136
caseClauseTypeNames.AddRange(types);
123137
}
124138
}
125139
}
126140

127-
if (TryDetermineEvaluationTypeFromTypes(caseClauseTypeNames, out _evalTypeName))
128-
{
129-
return _evalTypeName;
130-
}
131-
132-
caseClauseTypeNames.Remove(Tokens.Variant);
133-
if (TryDetermineEvaluationTypeFromTypes(caseClauseTypeNames, out _evalTypeName))
141+
if (TryDetermineEvaluationTypeFromTypes(caseClauseTypeNames, out string evalTypeName))
134142
{
135-
return _evalTypeName;
143+
return evalTypeName;
136144
}
137145
return string.Empty;
138146
}

‎Rubberduck.Inspections/Concrete/UnreachableCaseInspection/UnreachableCaseInspection.cs

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ public sealed class UnreachableCaseInspection : ParseTreeInspectionBase
1818
private IParseTreeValueVisitorFactory _parseTreeVisitorFactory;
1919
private ISelectCaseStmtContextWrapperFactory _selectStmtFactory;
2020
private IParseTreeValueFactory _valueFactory;
21-
private IParseTreeValueVisitor _parseTreeValueVisitor;
2221
private enum CaseInpectionResult { Unreachable, MismatchType, CaseElse };
2322

2423
private static Dictionary<CaseInpectionResult, string> ResultMessages = new Dictionary<CaseInpectionResult, string>()
@@ -36,23 +35,23 @@ public UnreachableCaseInspection(RubberduckParserState state) : base(state)
3635
_selectStmtFactory = factoriesFactory.CreateISelectStmtContextWrapperFactory();
3736
_valueFactory = factoriesFactory.CreateIParseTreeValueFactory();
3837
_parseTreeVisitorFactory = factoriesFactory.CreateIParseTreeValueVisitorFactory();
39-
40-
_parseTreeValueVisitor = _parseTreeVisitorFactory.Create(State, _valueFactory);
4138
}
4239

4340
public override IInspectionListener Listener { get; } =
4441
new UnreachableCaseInspectionListener();
4542

4643
private List<IInspectionResult> InspectionResults { set; get; } = new List<IInspectionResult>();
4744

48-
private ParseTreeVisitorResults ValueResults { get; } = new ParseTreeVisitorResults();
45+
private ParseTreeVisitorResults ValueResults { set; get; } = new ParseTreeVisitorResults();
4946

5047
protected override IEnumerable<IInspectionResult> DoGetInspectionResults()
5148
{
49+
InspectionResults = new List<IInspectionResult>();
5250
var qualifiedSelectCaseStmts = Listener.Contexts
5351
.Where(result => !IsIgnoringInspectionResultFor(result.ModuleName, result.Context.Start.Line));
5452

5553
var parseTreeValueVisitor = _parseTreeVisitorFactory.Create(State, _valueFactory);
54+
ValueResults = new ParseTreeVisitorResults();
5655
parseTreeValueVisitor.OnValueResultCreated += ValueResults.OnNewValueResult;
5756

5857
foreach (var qualifiedSelectCaseStmt in qualifiedSelectCaseStmts)

0 commit comments

Comments
 (0)