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

Cleanup of lightup layer after MS.CA upgrade #6762

Merged
merged 1 commit into from
Jul 13, 2023

Conversation

mavasani
Copy link
Contributor

Follow-up to #6726

@mavasani
Copy link
Contributor Author

@Youssef1313

Comment on lines -70 to -75
<Compile Include="$(MSBuildThisFileDirectory)Lightup\ITypeSymbolExtensions.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Lightup\IFunctionPointerInvocationOperationWrapper.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Lightup\LightupHelpers.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Lightup\NullableAnnotation.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Lightup\NullableContext.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Lightup\NullableContextExtensions.cs" />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed files moved to PublicApiAnalyzers project, which is still on MS.CA 1.2.1

Copy link
Member

Choose a reason for hiding this comment

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

1.2.1? That is Visual Studio 2015? Do we still care about such an old version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

There isn't any motivating need to update. It works fine and there is no indication that will change.

break;
}
case OperationKind.BinaryPattern:
// These high level patterns should not be present in the lowered CFG: https://github.com/dotnet/roslyn/issues/47068
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the root cause for #6755. We will need to figure out performing flow analysis in presence of binary pattern operations. I am working on a separate PR for it.

Copy link
Member

Choose a reason for hiding this comment

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

It could be for a bunch of CA1508 too. I'd love to see the correct handling of this will be!

@codecov
Copy link

codecov bot commented Jul 13, 2023

Codecov Report

Merging #6762 (8d8f5e0) into main (0d3a2bb) will increase coverage by 0.00%.
The diff coverage is 88.88%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6762   +/-   ##
=======================================
  Coverage   96.33%   96.33%           
=======================================
  Files        1389     1388    -1     
  Lines      325745   325717   -28     
  Branches    10739    10734    -5     
=======================================
- Hits       313809   313792   -17     
+ Misses       9224     9215    -9     
+ Partials     2712     2710    -2     

Copy link
Member

@Youssef1313 Youssef1313 left a comment

Choose a reason for hiding this comment

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

Looking good!

@mavasani mavasani marked this pull request as ready for review July 13, 2023 13:23
@mavasani mavasani requested a review from a team as a code owner July 13, 2023 13:23
@mavasani mavasani merged commit 4fe627a into dotnet:main Jul 13, 2023
16 checks passed
Comment on lines -1586 to -1587
// TODO: Remove the below string based checks when we move to Microsoft.CodeAnalysis 3.7 or later.
// TODO: File a tracking bug.
Copy link
Member

Choose a reason for hiding this comment

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

Ouch, this would have worked totally fine with lightup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants