Skip to content
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.

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

[Dogfooding] Enforce few more code style rules as build warnings in IDE projects #44164

Merged
merged 8 commits into from
May 12, 2020

Conversation

mavasani
Copy link
Contributor

Builds on top of #43394

# IDE0011: Add braces
csharp_prefer_braces = when_multiline:warning

# IDE0035: Remove unreachable code
dotnet_diagnostic.IDE0035.severity = warning

# IDE0043: Format string contains invalid placeholder
dotnet_diagnostic.IDE0043.severity = warning

# IDE0059: Unnecessary assignment to a value
dotnet_diagnostic.IDE0059.severity = warning

mavasani and others added 5 commits May 11, 2020 16:50
…warning for IDE projects (CodeStyle, Features and Workspaces layers)
# IDE0011: Add braces
csharp_prefer_braces = when_multiline:warning

# IDE0035: Remove unreachable code
dotnet_diagnostic.IDE0035.severity = warning

# IDE0059: Unnecessary assignment to a value
dotnet_diagnostic.IDE0059.severity = warning
… value (we do not offer code fixes to remove such assignments which could have side effects)
@mavasani mavasani added this to the 16.7.P2 milestone May 12, 2020
@mavasani mavasani requested review from sharwell, CyrusNajmabadi and a team May 12, 2020 00:18
@mavasani mavasani requested a review from a team as a code owner May 12, 2020 00:18
@mavasani mavasani changed the title [Dogfooding] Enforce few more code style rules in IDE projects [Dogfooding] Enforce few more code style rules as build warnings in IDE projects May 12, 2020
Copy link
Member

@cston cston left a comment

Choose a reason for hiding this comment

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

Compiler changes LGTM.

…is options collection in master branch, but my prior commit in this PR attempted to use it in the test. This leads the test to fail, so I have restored the original semantics of the test and removed an unused local.
@@ -106,7 +106,6 @@ public override Task RegisterCodeFixesAsync(CodeFixContext context)
var declaratorLocation = diagnostic.AdditionalLocations[0];
var identifierLocation = diagnostic.AdditionalLocations[1];
var invocationOrCreationLocation = diagnostic.AdditionalLocations[2];
var outArgumentContainingStatementLocation = diagnostic.AdditionalLocations[3];
Copy link
Member

Choose a reason for hiding this comment

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

shoudl probably fix the analzer to not provide this either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@CyrusNajmabadi addressed in d256ecd

// Invocations definitely have side effects. So we assume this
// is invalid initially;
expressionType = ExpressionType.Invalid;
#pragma warning restore IDE0059 // Unnecessary assignment of a value
Copy link
Member

Choose a reason for hiding this comment

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

why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am actually not sure if this would cause any side effects, especially given the comment. Just playing safe.

@@ -33,7 +33,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Simplification
Public Sub Initialize(parseOptions As ParseOptions, optionSet As OptionSet, cancellationToken As CancellationToken) Implements IReductionRewriter.Initialize
Me.ParseOptions = DirectCast(parseOptions, VisualBasicParseOptions)
_simplificationOptions = optionSet
cancellationToken = cancellationToken
Me.CancellationToken = cancellationToken
Copy link
Member

Choose a reason for hiding this comment

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

whoops!

@mavasani mavasani merged commit 7fa4e05 into dotnet:master May 12, 2020
@ghost ghost modified the milestones: 16.7.P2, Next May 12, 2020
@mavasani mavasani deleted the EnforceMoreCodeStyle_2 branch May 12, 2020 21:30
mavasani added a commit to mavasani/roslyn that referenced this pull request May 12, 2020
mavasani added a commit to mavasani/roslyn that referenced this pull request May 13, 2020
… IDE projects

My prior attempt to enable this as a build warning in dotnet#44164 did not work due to dotnet#44201. This PR actually adds the build enforcement and fixes the violations.
@JoeRobich JoeRobich removed this from the Next milestone May 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants