Skip to content

Take BugFixes to Master #229

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jun 1, 2015
Merged

Take BugFixes to Master #229

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
24d4515
Fix a bug in the flow graphs for throw statements
May 29, 2015
be47a8e
Merge pull request #227 from PowerShell/FixAvoidUninitializedVariable…
May 29, 2015
8b973b9
Simplifies logic of suppressing rule and fix a bug where rule suppres…
May 29, 2015
11507fc
Merge pull request #231 from PowerShell/FixRuleSuppressionErrorsForMo…
May 29, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
108 changes: 35 additions & 73 deletions Engine/Helper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
127 changes: 92 additions & 35 deletions Engine/VariableAnalysisBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -1877,7 +1917,7 @@ public Block Exit
/// </summary>
public void Init()
{
_entryBlock = new Block();
_entryBlock = Block.NewEntryBlock();
_exitBlock = new Block();
}

Expand Down Expand Up @@ -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++)
{
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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>
Expand Down Expand Up @@ -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;
}

Expand Down
9 changes: 9 additions & 0 deletions Tests/Engine/RuleSuppression.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
19 changes: 18 additions & 1 deletion Tests/Rules/AvoidGlobalOrUnitializedVarsNoViolations.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,4 +41,21 @@ function Test-PreferenceVariable
}

$VerbosePreference
}
}

function Test-Throw
{
if ($true)
{
throw "First time"
}

$a = 4

if ($false)
{
throw "Second time"
}

$a
}
Expand Down