Skip to content
This repository has been archived by the owner on Jan 24, 2021. It is now read-only.

Setup code analysis #2535

Merged
merged 2 commits into from
Nov 1, 2016
Merged

Setup code analysis #2535

merged 2 commits into from
Nov 1, 2016

Conversation

xt0rted
Copy link
Contributor

@xt0rted xt0rted commented Aug 13, 2016

This is based on the discussion in #2534 and sets up code analysis with a custom ruleset that includes DotNetAnalyzers/AsyncUsageAnalyzers. All the other rules are turned off right now.

In the classic project setup you can view the code analysis rules in the project's properties and when building the errors will show in the Error List.

In the new project setup there's no code analysis settings in the project's properties, but it does allow you to detect and run the rules. You do that by right clicking on the solution/folder/project and selecting Find Code Issues which should bring up the Inspection Results window which is similar to the old Error List.

I'm not sure if the new project setup can run these on build or in a separate step, or if we'll have to wait for the rollback to msbuild for that.

The results of this show the only issue being in NancyHost.cs#L133-L134 as per #2471

@jchannon
Copy link
Member

Is this whole thing a VS feature ie. Can you get analysis from dotnet build?

On Saturday, 13 August 2016, Brian Surowiec notifications@github.com
wrote:

This is based on the discussion in #2534
#2534 and sets up code analysis
with a custom ruleset that includes DotNetAnalyzers/AsyncUsageAnalyzers
https://github.com/DotNetAnalyzers/AsyncUsageAnalyzers. All the other
rules are turned off right now.

In the classic project setup you can view the code analysis rules in the
project's properties and when building the errors will show in the Error
List.

In the new project setup there's no code analysis settings in the
project's properties, but it does allow you to detect and run the rules.
You do that by right clicking on the solution/folder/project and selecting Find
Code Issues which should bring up the Inspection Results window which is
similar to the old Error List.

I'm not sure if the new project setup can run these on build or in a
separate step, or if we'll have to wait for the rollback to msbuild for
that.

The results of this show the only issue being in NancyHost.cs#L133-L134

Nancy/src/Nancy.Hosting.Self/NancyHost.cs

Lines 133 to 134 in d76e7ef

HttpListenerContext context = await listener.GetContextAsync();
Process(context);

as per #2471 #2471

You can view, comment on, or merge this pull request online at:

#2535
Commit Summary

  • Add code analysis including async rules
  • Add async code analysis

File Changes

Patch Links:

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#2535, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAGapritXSnkHmcst9il5doVBgLClO2Pks5qfcSLgaJpZM4JjrRj
.

@xt0rted
Copy link
Contributor Author

xt0rted commented Aug 13, 2016

I just turned on CS4014 and that's giving two new warnings. One is in NancyHost.cs, the other is DiagnosticsHook.cs#L186

@jchannon the msbuild output has these warnings and errors, you can see them in the teamcity build log (search for CS4014). I'm not sure why the async usage ones I just added aren't showing up there though.

I'm also not sure about dotnet build since I've been waiting for them to stop changing every release before I started looking into it.

@jchannon
Copy link
Member

But interestingly it doesn't show an issue around the code in the PR that
@khellang did

On Saturday, 13 August 2016, Brian Surowiec notifications@github.com
wrote:

I just turned on CS4014
https://msdn.microsoft.com/en-us/library/hh873131.aspx and that's
giving two new warnings. One is in NancyHost.cs, the other is
DiagnosticsHook.cs#L186

Nancy/src/Nancy/Diagnostics/DiagnosticsHook.cs

Line 186 in d76e7ef

resolveResult.After.Invoke(ctx, CancellationToken);

@jchannon https://github.com/jchannon the msbuild output has these
warnings and errors, you can see them in the teamcity build log
http://nancy-ci.cloudapp.net/viewLog.html?buildId=7476&buildTypeId=Nancy_Pulls&tab=buildLog#_state=47,177&focus=497
(search for CS4014). I'm not sure why the async usage ones I just added
aren't showing up there though.

I'm also not sure about dotnet build since I've been waiting for them to
stop changing every release before I started looking into it.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#2535 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAGapsZLbu8NLIwDYl-1ZsOyKmNsLQ81ks5qfc5vgaJpZM4JjrRj
.

@xt0rted
Copy link
Contributor Author

xt0rted commented Aug 13, 2016

It looks like that code isn't being detected by these rules because of how it's invoked. Since it's in a constructor and is using .Wait() instead of await you can't call .ConfigureAwait(false) so it's not being flagged.

@xt0rted
Copy link
Contributor Author

xt0rted commented Aug 13, 2016

After looking into this some more it seems the analyzers don't work with .net core. There's a number of issues about this accross the various dotnet & roslyn projects which seem to indicate it's a tooling issue and/or clr issue and should be fixed in the 1.1 release.

@thecodejunkie
Copy link
Member

So what do we want to do with this?

@thecodejunkie thecodejunkie added this to the 2.0-clinteastwood milestone Aug 30, 2016
@thecodejunkie
Copy link
Member

@xt0rted is this still in limbo? If so then we'll push it forward to the next milestone. If not then it needs rebasing (and updating?) so we can pull it in. Trying to wrap up the two remaining issues on the -clinteastwood milestone :)

@xt0rted
Copy link
Contributor Author

xt0rted commented Sep 28, 2016 •
edited
Loading

@thecodejunkie this can wait, I need to look into why the build is failing after my rebase but I don't really have the time right now.

Update: looks like the build is failing, at least partly, due to the recent AngleSharp update. #2578 should fix that.

@xt0rted xt0rted mentioned this pull request Sep 28, 2016
Merged
4 tasks
@xt0rted
Copy link
Contributor Author

xt0rted commented Oct 31, 2016

@thecodejunkie rebased and passing

@thecodejunkie thecodejunkie merged commit 10318e6 into NancyFx:master Nov 1, 2016
@xt0rted xt0rted deleted the code-analysis branch November 1, 2016 14:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.0-clinteastwood
Development

Successfully merging this pull request may close these issues.
3 participants
@xt0rted @jchannon @thecodejunkie