diff --git a/Engine/Helper.cs b/Engine/Helper.cs index 94bc8b871..373f4bd58 100644 --- a/Engine/Helper.cs +++ b/Engine/Helper.cs @@ -859,102 +859,64 @@ public Tuple, List> SuppressRule(string List ruleSuppressions = ruleSuppressionsDict[ruleName]; int recordIndex = 0; - int ruleSuppressionIndex = 0; - DiagnosticRecord record = diagnostics.First(); - RuleSuppression ruleSuppression = ruleSuppressions.First(); - int suppressionCount = 0; + int startRecord = 0; + bool[] suppressed = new bool[diagnostics.Count]; - while (recordIndex < diagnostics.Count) + foreach (RuleSuppression ruleSuppression in ruleSuppressions) { - if (!String.IsNullOrWhiteSpace(ruleSuppression.Error)) + int suppressionCount = 0; + while (startRecord < diagnostics.Count && diagnostics[startRecord].Extent.StartOffset < ruleSuppression.StartOffset) { - ruleSuppressionIndex += 1; + startRecord += 1; + } + + // at this point, start offset of startRecord is greater or equals to rulesuppression.startoffset + recordIndex = startRecord; + + while (recordIndex < diagnostics.Count) + { + DiagnosticRecord record = diagnostics[recordIndex]; - if (ruleSuppressionIndex == ruleSuppressions.Count) + if (record.Extent.EndOffset > ruleSuppression.EndOffset) { break; } - ruleSuppression = ruleSuppressions[ruleSuppressionIndex]; - suppressionCount = 0; - - continue; - } - - // if the record precedes the rule suppression then we don't apply the suppression - // so we check that start of record is greater than start of suppression - if (record.Extent.StartOffset >= ruleSuppression.StartOffset) - { - // end of the rule suppression is less than the record start offset so move on to next rule suppression - if (ruleSuppression.EndOffset < record.Extent.StartOffset) + if (string.IsNullOrWhiteSpace(ruleSuppression.RuleSuppressionID)) { - ruleSuppressionIndex += 1; - - // If we cannot found any error but the rulesuppression has a rulesuppressionid then it must be used wrongly - if (!String.IsNullOrWhiteSpace(ruleSuppression.RuleSuppressionID) && suppressionCount == 0) - { - ruleSuppression.Error = String.Format(CultureInfo.CurrentCulture, Strings.RuleSuppressionErrorFormat, ruleSuppression.StartAttributeLine, - System.IO.Path.GetFileName(record.Extent.File), String.Format(Strings.RuleSuppressionIDError, ruleSuppression.RuleSuppressionID)); - Helper.Instance.MyCmdlet.WriteError(new ErrorRecord(new ArgumentException(ruleSuppression.Error), ruleSuppression.Error, ErrorCategory.InvalidArgument, ruleSuppression)); - } - - if (ruleSuppressionIndex == ruleSuppressions.Count) - { - break; - } - - ruleSuppression = ruleSuppressions[ruleSuppressionIndex]; - suppressionCount = 0; - - continue; + suppressed[recordIndex] = true; + suppressionCount += 1; } - // at this point, the record is inside the interval else { - // if the rule suppression id from the rule suppression is not null and the one from diagnostic record is not null - // and they are they are not the same then we cannot ignore the record - if (!string.IsNullOrWhiteSpace(ruleSuppression.RuleSuppressionID) && !string.IsNullOrWhiteSpace(record.RuleSuppressionID) - && !string.Equals(ruleSuppression.RuleSuppressionID, record.RuleSuppressionID, StringComparison.OrdinalIgnoreCase)) - { - suppressionCount -= 1; - unSuppressedRecords.Add(record); - } - // otherwise, we suppress the record, move on to the next. - else + //if there is a rule suppression id, we only suppressed if it matches + if (!String.IsNullOrWhiteSpace(record.RuleSuppressionID) && + string.Equals(ruleSuppression.RuleSuppressionID, record.RuleSuppressionID, StringComparison.OrdinalIgnoreCase)) { + suppressed[recordIndex] = true; suppressedRecords.Add(new SuppressedRecord(record, ruleSuppression)); + suppressionCount += 1; } } - } - else - { - unSuppressedRecords.Add(record); - } - // important assumption: this point is reached only if we want to move to the next record - recordIndex += 1; - suppressionCount += 1; + recordIndex += 1; + } - if (recordIndex == diagnostics.Count) + // If we cannot found any error but the rulesuppression has a rulesuppressionid then it must be used wrongly + if (!String.IsNullOrWhiteSpace(ruleSuppression.RuleSuppressionID) && suppressionCount == 0) { - // If we cannot found any error but the rulesuppression has a rulesuppressionid then it must be used wrongly - if (!String.IsNullOrWhiteSpace(ruleSuppression.RuleSuppressionID) && suppressionCount == 0) - { - ruleSuppression.Error = String.Format(CultureInfo.CurrentCulture, Strings.RuleSuppressionErrorFormat, ruleSuppression.StartAttributeLine, - System.IO.Path.GetFileName(record.Extent.File), String.Format(Strings.RuleSuppressionIDError, ruleSuppression.RuleSuppressionID)); - Helper.Instance.MyCmdlet.WriteError(new ErrorRecord(new ArgumentException(ruleSuppression.Error), ruleSuppression.Error, ErrorCategory.InvalidArgument, ruleSuppression)); - } - - break; + ruleSuppression.Error = String.Format(CultureInfo.CurrentCulture, Strings.RuleSuppressionErrorFormat, ruleSuppression.StartAttributeLine, + System.IO.Path.GetFileName(diagnostics.First().Extent.File), String.Format(Strings.RuleSuppressionIDError, ruleSuppression.RuleSuppressionID)); + Helper.Instance.MyCmdlet.WriteError(new ErrorRecord(new ArgumentException(ruleSuppression.Error), ruleSuppression.Error, ErrorCategory.InvalidArgument, ruleSuppression)); } - - record = diagnostics[recordIndex]; } - while (recordIndex < diagnostics.Count) + for (int i = 0; i < suppressed.Length; i += 1) { - unSuppressedRecords.Add(diagnostics[recordIndex]); - recordIndex += 1; + if (!suppressed[i]) + { + unSuppressedRecords.Add(diagnostics[i]); + } } return result; diff --git a/Engine/VariableAnalysisBase.cs b/Engine/VariableAnalysisBase.cs index 272699471..f1d40ca66 100644 --- a/Engine/VariableAnalysisBase.cs +++ b/Engine/VariableAnalysisBase.cs @@ -405,15 +405,36 @@ public class Block internal bool _returns; internal bool _unreachable; + // Only Entry block, that can be constructed via NewEntryBlock() is reachable initially. + // all other blocks are unreachable. + // reachability of block should be proved with FlowsTo() calls. + public Block() + { + this._unreachable = true; + } + + public static Block NewEntryBlock() + { + return new Block(unreachable: false); + } + + private Block(bool unreachable) + { + this._unreachable = unreachable; + } + + /// + /// Tell flow analysis that this block can flow to next block. + /// + /// internal void FlowsTo(Block next) { if (_successors.IndexOf(next) < 0) { - if (_unreachable) + if (!_unreachable) { - next._unreachable = true; + next._unreachable = false; } - _successors.Add(next); next._predecessors.Add(this); } @@ -725,6 +746,25 @@ internal static void RenameVariables(Block block) internal static void InitializeSSA(Dictionary VariableAnalysis, Block Entry, List Blocks) { VariablesDictionary = new Dictionary(StringComparer.OrdinalIgnoreCase); + + foreach (var block in Blocks) + { + List _unreachables = new List(); + foreach (var pred in block._predecessors) + { + if (pred._unreachable) + { + _unreachables.Add(pred); + pred._successors.Remove(block); + } + } + + foreach (var pred in _unreachables) + { + block._predecessors.Remove(pred); + } + } + InternalVariablesDictionary.Clear(); SSADictionary.Clear(); Counters.Clear(); @@ -1877,7 +1917,7 @@ public Block Exit /// public void Init() { - _entryBlock = new Block(); + _entryBlock = Block.NewEntryBlock(); _exitBlock = new Block(); } @@ -2057,6 +2097,13 @@ public object VisitIfStatement(IfStatementAst ifStmtAst) if (ifStmtAst == null) return null; Block afterStmt = new Block(); + + if (ifStmtAst.ElseClause == null) + { + // There is no else, flow can go straight to afterStmt. + _currentBlock.FlowsTo(afterStmt); + } + int clauseCount = ifStmtAst.Clauses.Count; for (int i = 0; i < clauseCount; i++) { @@ -2403,52 +2450,62 @@ public object VisitTryStatement(TryStatementAst tryStatementAst) tryStatementAst.Body.Visit(this.Decorator); - // This is either the first block in the finally, or the first block after all the catches if there is no finally. - // For return analysis, we start off assuming this block is reachable only if the end of the try body - // is reachable, but that can change if we find a catch that falls through. - var afterTry = new Block { _unreachable = _currentBlock._unreachable }; + Block lastBlockInTry = _currentBlock; + var finallyFirstBlock = tryStatementAst.Finally == null ? null : new Block(); + Block finallyLastBlock = null; + + // This is the first block after all the catches and finally (if present). + var afterTry = new Block(); + + bool isCatchAllPresent = false; foreach (var catchAst in tryStatementAst.CatchClauses) { + if (catchAst.IsCatchAll) + { + isCatchAllPresent = true; + } + // Any statement in the try block could throw and reach the catch, so assume the worst (from a data // flow perspective) and make the predecessor to the catch the block before entering the try. _currentBlock = new Block(); blockBeforeTry.FlowsTo(_currentBlock); - catchAst.Visit(this.Decorator); - - if (!_currentBlock._unreachable) - { - // The last block of the catch falls through, so we can get reach blocks after the try. - afterTry._unreachable = false; - } + _currentBlock.FlowsTo(finallyFirstBlock ?? afterTry); } - // Assume nothing in the try was executed, so flow comes from before the try. We could have the catch blocks - // also flow to this block, but that won't improve the analysis any, so skip that. - _currentBlock = afterTry; - blockBeforeTry.FlowsTo(_currentBlock); - - if (tryStatementAst.Finally != null) + if (finallyFirstBlock != null) { + lastBlockInTry.FlowsTo(finallyFirstBlock); + + _currentBlock = finallyFirstBlock; tryStatementAst.Finally.Visit(this.Decorator); + _currentBlock.FlowsTo(afterTry); + + finallyLastBlock = _currentBlock; - if (!_currentBlock._unreachable) + // For finally block, there are 2 cases: when try-body throw and when it doesn't. + // For these two cases value of 'finallyLastBlock._throws' would be different. + if (!isCatchAllPresent) { - // If the finally throws (it can't have other flow like return, break, or continue) - // then after the try/catch/finally is unreachable, otherwise it is reachable. - afterTry._unreachable = false; + // This flow exist only, if there is no catch for all exceptions. + blockBeforeTry.FlowsTo(finallyFirstBlock); + + var rethrowAfterFinallyBlock = new Block(); + finallyLastBlock.FlowsTo(rethrowAfterFinallyBlock); + rethrowAfterFinallyBlock._throws = true; + rethrowAfterFinallyBlock.FlowsTo(_exitBlock); } - // We can assume that flow from the finally reaches the code following the finally. Of course, if an exception occurs, - // then the code can't be reached, but our conservative data flow modeling assures us correctness, the exception will - // either leave the current function (meaning it doesn't matter that we assumed flow reached after the finally), or - // it will be caught and handled by a containing catch, and our conversative data flow ensures the correctness of the - // generated code. - var newBlock = new Block(); - _currentBlock.FlowsTo(newBlock); - _currentBlock = newBlock; + // This flow always exists. + finallyLastBlock.FlowsTo(afterTry); } + else + { + lastBlockInTry.FlowsTo(afterTry); + } + + _currentBlock = afterTry; return null; } @@ -2486,7 +2543,7 @@ where t.Label.Equals(labelStrAst.Value, StringComparison.OrdinalIgnoreCase) } // The next block is unreachable, but is necessary to keep the flow graph correct. - _currentBlock = new Block { _unreachable = true }; + _currentBlock = new Block(); } /// @@ -2525,7 +2582,7 @@ internal Block ControlFlowStatement(PipelineBaseAst pipelineAst) var lastBlockInStatement = _currentBlock; // The next block is unreachable, but is necessary to keep the flow graph correct. - _currentBlock = new Block { _unreachable = true }; + _currentBlock = new Block(); return lastBlockInStatement; } diff --git a/Tests/Engine/RuleSuppression.ps1 b/Tests/Engine/RuleSuppression.ps1 index 45ae97bd3..d2fc8e485 100644 --- a/Tests/Engine/RuleSuppression.ps1 +++ b/Tests/Engine/RuleSuppression.ps1 @@ -14,6 +14,15 @@ function SuppressMe () } +function SuppressTwoVariables() +{ + [System.Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSAvoidUninitializedVariable", "b")] + [System.Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSAvoidUninitializedVariable", "a")] + Param([string]$a, [int]$b) + { + } +} + [System.Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSAvoidUsingConvertToSecureStringWithPlainText", "", Scope="Class")] [System.Diagnostics.CodeAnalysis.SuppressMessageAttribute("pSAvoidUsingInvokeExpression", "")] class TestClass diff --git a/Tests/Rules/AvoidGlobalOrUnitializedVarsNoViolations.ps1 b/Tests/Rules/AvoidGlobalOrUnitializedVarsNoViolations.ps1 index 6a9ce8077..42c137872 100644 --- a/Tests/Rules/AvoidGlobalOrUnitializedVarsNoViolations.ps1 +++ b/Tests/Rules/AvoidGlobalOrUnitializedVarsNoViolations.ps1 @@ -41,4 +41,21 @@ function Test-PreferenceVariable } $VerbosePreference - } \ No newline at end of file +} + +function Test-Throw +{ + if ($true) + { + throw "First time" + } + + $a = 4 + + if ($false) + { + throw "Second time" + } + + $a +} \ No newline at end of file