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.

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

Jump to bottom

VSTHRD114: Do not return null from non-async Task method #596

Merged
merged 14 commits into from
Apr 19, 2020
Merged

VSTHRD114: Do not return null from non-async Task method #596

merged 14 commits into from
Apr 19, 2020

Conversation

Evangelink
Copy link
Member

Fix #593

@msftclas
Copy link

msftclas commented Mar 27, 2020 •
edited
Loading

CLA assistant check
All CLA requirements met.

AArnott
AArnott reviewed Mar 28, 2020
View reviewed changes
src/Microsoft.VisualStudio.Threading.Analyzers/Utils.cs Outdated
catch (Exception ex) when (LaunchDebuggerExceptionFilter())
{
var messageBuilder = new StringBuilder();
messageBuilder.AppendLine("Analyzer failure while processing syntax at");
Copy link
Member

Choose a reason for hiding this comment

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

Do multi-line exception messages get printed alright at all the interesting points? (e.g. command line builds, VS error list, infobar, etc.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Honestly I don't know and I don't know all contexts I will need to test so I will move to a single-line message. From my tests it is ok with infobar and error list.

src/Microsoft.VisualStudio.Threading.Analyzers/Strings.resx Outdated Show resolved Hide resolved
src/Microsoft.VisualStudio.Threading.Analyzers/Strings.resx Outdated Show resolved Hide resolved
src/Microsoft.VisualStudio.Threading.Analyzers.Tests/MultiAnalyzerTests.cs Outdated Show resolved Hide resolved
...oft.VisualStudio.Threading.Analyzers/VSTHRD112AvoidNullReturnInNonAsyncTaskMethodAnalyzer.cs Outdated Show resolved Hide resolved
...oft.VisualStudio.Threading.Analyzers/VSTHRD112AvoidNullReturnInNonAsyncTaskMethodAnalyzer.cs Outdated Show resolved Hide resolved
...tudio.Threading.Analyzers.Tests/VSTHRD112AvoidNullReturnInNonAsyncTaskMethodAnalyzerTests.cs Outdated Show resolved Hide resolved
...tudio.Threading.Analyzers.Tests/VSTHRD112AvoidNullReturnInNonAsyncTaskMethodAnalyzerTests.cs Outdated Show resolved Hide resolved
AArnott
AArnott requested changes Mar 28, 2020
View reviewed changes
Copy link
Member

@AArnott AArnott left a comment

Choose a reason for hiding this comment

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

Great contribution. Thank you! I like your set of tests as well, and your use of Operation for a very simple analyzer is great!

...tudio.Threading.Analyzers.Tests/VSTHRD112AvoidNullReturnInNonAsyncTaskMethodAnalyzerTests.cs Outdated Show resolved Hide resolved
...oft.VisualStudio.Threading.Analyzers/VSTHRD112AvoidNullReturnInNonAsyncTaskMethodAnalyzer.cs Outdated
/// Finds await expressions on <see cref="Task"/> that do not use <see cref="Task.ConfigureAwait(bool)"/>.
/// Also works on <see cref="ValueTask"/>.
/// </summary>
[DiagnosticAnalyzer(LanguageNames.CSharp)]
Copy link
Member

Choose a reason for hiding this comment

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

Can you add VB as a supported language here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you want me to introduce vbnet tests?

Copy link
Member

Choose a reason for hiding this comment

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

Just one or two sanity tests in VB would be great, yes.

@Evangelink Evangelink requested a review from AArnott April 2, 2020 08:12
@Evangelink
Copy link
Member Author

@AArnott I have added 2 code fix for the rule. I don't have any answer from @sharwell regarding the use of lightup so I am not sure I will be able to have the nullable enable context handled. If that's ok for you, I suggest that we move forward with this PR and I soon as I get an answer I take care of adding a new PR to handle the nullable context.

sharwell
sharwell suggested changes Apr 3, 2020
View reviewed changes
Copy link
Member

@sharwell sharwell left a comment •
edited
Loading

Choose a reason for hiding this comment

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

THIRD-PARTY-NOTICES.txt needs to be updated with this content (for the lightup layer):

https://github.com/dotnet/roslyn-analyzers/blob/2fa9764aaffeea3e14be4dd8a1ede5d14026dd49/THIRD-PARTY-NOTICES.txt#L43-L68

@Evangelink @sharwell
Apply suggestions from code review
3ec3615 
Co-Authored-By: Sam Harwell <sam@tunnelvisionlabs.com>
Evangelink
Evangelink commented Apr 3, 2020
View reviewed changes
src/Microsoft.VisualStudio.Threading.Analyzers/VSTHRD112AvoidReturningNullTaskAnalyzer.cs Outdated
returnOperation.ReturnedValue.ConstantValue.HasValue &&
returnOperation.ReturnedValue.ConstantValue.Value == null &&
returnOperation.ReturnedValue.Syntax is { } returnedValueSyntax &&
!context.Compilation.GetSemanticModel(returnedValueSyntax.SyntaxTree).GetNullableContext(returnedValueSyntax.SpanStart).AnnotationsEnabled())
Copy link
Member Author

Choose a reason for hiding this comment

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

I did something similar on roslyn-analyzers and I end-up with a RS1030 so I think we shouldn't be moving forward like this. @sharwell any idea of how I could rework this? On dotnet/roslyn-analyzers#3352 we decided to rewrite the analyzer as syntax analyzer but that seems more complex in here.

Copy link
Member Author

Choose a reason for hiding this comment

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

@sharwell @AArnott I have tried a couple of things and I cannot find any good way (except for a full node analyzer) to get the semantic model using the operation analyzer. I am not sure how to move forward:

1/ Revert the part trying to handle the #nullable enable part.

2/ Keep current implementation. That's probably a bad idea as an analyzer was written to prevent this case.

3/ Move to a syntax analyzer.

4/ Any other way?

Copy link
Member

Choose a reason for hiding this comment

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

Personally I think we can drop no. 2.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's ok when bumping the version of roslyn we are using.

@Evangelink Evangelink changed the title VSTHRD112: Do not return null from non-async Task method VSTHRD114: Do not return null from non-async Task method Apr 16, 2020
Evangelink
Evangelink commented Apr 16, 2020
View reviewed changes
...sualStudio.Threading.Analyzers.Tests/Microsoft.VisualStudio.Threading.Analyzers.Tests.csproj Outdated
@@ -23,7 +23,7 @@
<PackageReference Include="MicroBuild.VisualStudio" Version="$(MicroBuildVersion)" PrivateAssets="all" />
<PackageReference Include="Microsoft.CodeAnalysis.CSharp.CodeFix.Testing.XUnit" Version="1.0.1-beta1.20209.1" />
<PackageReference Include="Microsoft.CodeAnalysis.VisualBasic.CodeFix.Testing.XUnit" Version="1.0.1-beta1.20209.1" />
<PackageReference Include="Microsoft.CodeAnalysis" Version="2.8.2" />
<PackageReference Include="Microsoft.CodeAnalysis" Version="3.1.0" />
Copy link
Member Author

Choose a reason for hiding this comment

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

Min version where C# 8 language exists.

Evangelink
Evangelink commented Apr 16, 2020
View reviewed changes
Copy link
Member Author

@Evangelink Evangelink left a comment

Choose a reason for hiding this comment

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

Please do a full review again.

@Evangelink
Copy link
Member Author

For some reason now the tests are considered as a nullable disabled context...

@AArnott
Remove light-ups to get tests working
d827da7 
I don't so much care that someone with NRT will get duplicate diagnostics when returning null.
AArnott
AArnott approved these changes Apr 18, 2020
View reviewed changes
Copy link
Member

@AArnott AArnott left a comment

Choose a reason for hiding this comment

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

Thank you!

@AArnott AArnott merged commit 6c5108f into microsoft:master Apr 19, 2020
@Evangelink Evangelink deleted the task-return-null branch April 19, 2020 06:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AArnott AArnott AArnott approved these changes

@sharwell sharwell Awaiting requested review from sharwell

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Analyzer proposal: Do not return null from non-async Task method
4 participants
@Evangelink @msftclas @AArnott @sharwell