Merge pull request #229 from PowerShell/BugFixes

Take BugFixes to Master

Author: raghushantha

Engine/Helper.cs

@@ -859,102 +859,64 @@ public Tuple<List<SuppressedRecord>, List<DiagnosticRecord>> SuppressRule(string
             List<RuleSuppression> 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;

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;
+        }
+
+        /// <summary>
+        /// Tell flow analysis that this block can flow to next block.
+        /// </summary>
+        /// <param name="next"></param>
         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<string, VariableAnalysisDetails> VariableAnalysis, Block Entry, List<Block> Blocks)
         {
             VariablesDictionary = new Dictionary<string, VariableAnalysisDetails>(StringComparer.OrdinalIgnoreCase);
+
+            foreach (var block in Blocks)
+            {
+                List<Block> _unreachables = new List<Block>();
+                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
         /// </summary>
         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();
         }
 
         /// <summary>
@@ -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;
         }
 

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 

Tests/Rules/AvoidGlobalOrUnitializedVarsNoViolations.ps1

@@ -41,4 +41,21 @@ function Test-PreferenceVariable
         }
 
         $VerbosePreference
-    }
+}
+
+function Test-Throw
+{
+    if ($true)
+    {
+        throw "First time"
+    }
+
+    $a = 4
+
+    if ($false)
+    {
+        throw "Second time"
+    }
+
+    $a
+}