Skip to content

Conversation

@Youssef1313
Copy link
Member

@Youssef1313 Youssef1313 commented Jul 17, 2021

Closes #5251

@Youssef1313 Youssef1313 requested a review from a team as a code owner July 17, 2021 16:05
Comment on lines -20 to -25
public static bool IsPortedFxCopRule(DiagnosticDescriptor diagnosticDescriptor)
{
var result = diagnosticDescriptor.CustomTags.Any(t => t == PortedFromFxCop);
Debug.Assert(!result || diagnosticDescriptor.Id.StartsWith("CA", StringComparison.OrdinalIgnoreCase));
return result;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This is not currently used.

It may have been used in the deleted FxCopAnalyzers.sln.

@codecov
Copy link

codecov bot commented Jul 18, 2021

Codecov Report

Merging #5252 (e407040) into main (09f855d) will increase coverage by 0.01%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##             main    #5252      +/-   ##
==========================================
+ Coverage   95.56%   95.57%   +0.01%     
==========================================
  Files        1281     1281              
  Lines      296544   296504      -40     
  Branches    18096    18088       -8     
==========================================
- Hits       283378   283370       -8     
+ Misses      10705    10684      -21     
+ Partials     2461     2450      -11     

Copy link
Member

@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.

Nice cleanup @Youssef1313

public const string MicrosoftCodeAnalysisDiagnosticsSyntaxNodeAnalysisContext = "Microsoft.CodeAnalysis.Diagnostics.SyntaxNodeAnalysisContext";
public const string MicrosoftCodeAnalysisDiagnosticsSyntaxTreeAnalysisContext = "Microsoft.CodeAnalysis.Diagnostics.SyntaxTreeAnalysisContext";
public const string MicrosoftCodeAnalysisHostMefMefConstruction = "Microsoft.CodeAnalysis.Host.Mef.MefConstruction";
public const string MicrosoftCodeAnalysisLocalizableString = "Microsoft.CodeAnalysis.LocalizableString";
Copy link
Member

Choose a reason for hiding this comment

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

@mavasani Do you think the code style repo would accept a suggestion for an analyzer that sorts members by name? Shall we do something specific to WellKnownTypeNames?

Choose a reason for hiding this comment

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

@Evangelink I think StyleCop might already have such an analyzer. Feel free to open an issue on Roslyn otherwise.

Copy link
Member

Choose a reason for hiding this comment

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

Can we enable FxCop on this repo?

generator.InvocationExpression(
generator.MemberAccessExpression(
generator.TypeExpression(semanticModel.Compilation.GetSpecialType(SpecialType.System_Object)),
generator.TypeExpression(SpecialType.System_Object),
Copy link
Member

Choose a reason for hiding this comment

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

I guess that's a new API on roslyn side? I don't know why we would have gone through this more complex syntax otherwise.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Evangelink Both APIs were introduced in 3.0.0. I think this approach was taken since this is the first overload shown by IntelliSense?

@Youssef1313
Copy link
Member Author

Important before merging: This may regress respecting user preferences for CodeStyleOptions.PreferIntrinsicPredefinedTypeKeywordInMemberAccess.

Converting to a draft for now.

@Youssef1313 Youssef1313 marked this pull request as draft July 21, 2021 18:31
@mavasani
Copy link

@Youssef1313 Is this PR ready?

@mavasani
Copy link

mavasani commented Nov 3, 2021

Ping @Youssef1313

1 similar comment
@Evangelink
Copy link
Member

Ping @Youssef1313

@Youssef1313
Copy link
Member Author

Important before merging: This may regress respecting user preferences for CodeStyleOptions.PreferIntrinsicPredefinedTypeKeywordInMemberAccess.

Converting to a draft for now.

I haven't yet looked at this unfortunately.

@Evangelink
Copy link
Member

@Youssef1313 Thanks for the heads-up!

@Youssef1313
Copy link
Member Author

Youssef1313 commented Nov 18, 2021

For reference, dotnet/roslyn#55004 was what revealed the potential problem with this PR.

We can still take the small parts of this PR that doesn't use TypeExpression(SpecialType) overload. However, I think Roslyn should have respected user preferences for this overload. cc @sharwell @CyrusNajmabadi

I opened dotnet/roslyn#57867 on roslyn side.

@Youssef1313 Youssef1313 marked this pull request as ready for review November 19, 2021 10:12
@Youssef1313
Copy link
Member Author

@mavasani @Evangelink Sorry for the delay. I reverted the potentially problematic changes.

@mavasani mavasani merged commit a4ffd4f into dotnet:main Nov 22, 2021
@github-actions github-actions bot added this to the vNext milestone Nov 22, 2021
@Youssef1313 Youssef1313 deleted the cleanup branch November 22, 2021 06:17
@jmarolf jmarolf removed this from the vNext milestone Nov 15, 2022
@jmarolf jmarolf added this to the .NET 7.x milestone Nov 15, 2022
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.

Unreachable null check after call to GetSpecialType

4 participants