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

New Analyzer: Warn when calling Enumerable.Cast<T>/OfType<T> with incompatible types #4328

Merged
merged 7 commits into from
Jan 25, 2023

Conversation

fowl2
Copy link
Contributor

@fowl2 fowl2 commented Oct 14, 2020

Warn if use of Enumerable.Cast<T> would always fail at runtime or OfType<T>() would provably result in an empty sequence, because we can see that the input type in the sequence will never be the specified type.

RuleId: CA2021
Category: Reliability

related: #3608, dotnet/runtime#33770

  • Tests pass
  • Targeted to release/7.0.1xx main
  • Run against
    • dotnet/roslyn-analyzers
    • dotnet/runtime
    • dotnet/roslyn
  • Docs

@dnfadmin
Copy link

dnfadmin commented Oct 14, 2020

CLA assistant check
All CLA requirements met.

@fowl2
Copy link
Contributor Author

fowl2 commented Oct 14, 2020

That should be all the warnings resolved except for the rule id. Not sure how those are are assigned.

Copy link
Contributor

@buyaa-n buyaa-n left a comment

Choose a reason for hiding this comment

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

@fowl2 The analyzer proposal has not yet been approved, please wait until the proposal goes through API review

@carlossanlop
Copy link
Member

The proposal has been approved so we can unblock the PR, @buyaa-n

Copy link
Contributor

@buyaa-n buyaa-n left a comment

Choose a reason for hiding this comment

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

Overall looks good @fowl2 thank you for your contribution, added few comment, and seems we could add some more unit tests.
Not sure if you read our Definition of done list, it would be great if you could try testing the new analyzer with runtime repo, please let us know if you would like to give it a try

@mavasani
Copy link
Contributor

mavasani commented Dec 3, 2020

Copy link
Contributor

@mavasani mavasani left a comment

Choose a reason for hiding this comment

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

@buyaa-n
Copy link
Contributor

buyaa-n commented May 21, 2021

@fowl2 could you address the comments? Do you need a help with anything?

Also please retarget to https://github.com/dotnet/roslyn-analyzers/tree/release/6.0.1xx

@fowl2
Copy link
Contributor Author

fowl2 commented May 24, 2021

@buyaa-n sorry I haven't had time to look at this - looks like I'll have some spare time this week to get back to you

@danmoseley
Copy link
Member

@fowl2 could you give us an update? It would be great to have this analyzer. Do you expect to have more time?

@buyaa-n
Copy link
Contributor

buyaa-n commented Aug 13, 2021

ping @fowl2 again, are you planning to work on it?

@Youssef1313
Copy link
Member

@Youssef1313 do you have any hints as to how to run test against dotnet/roslyn-analyzers?

Can you try the instructions here?

@fowl2
Copy link
Contributor Author

fowl2 commented Sep 13, 2021

Yes that's what I was trying. See my previous comment :)

@Youssef1313
Copy link
Member

Probably @buyaa-n or @mavasani can help better here.

@jmarolf jmarolf changed the base branch from release/7.0.1xx to main September 28, 2021 22:03
@buyaa-n
Copy link
Contributor

buyaa-n commented Oct 21, 2021

do you have any hints as to how to run test against dotnet/roslyn-analyzers?

Sorry for the late reply @fowl2, apparently the repo's dogfooding analyzer is updated

<PackageReference Include="Microsoft.CodeAnalysis.NetAnalyzers" Version="$(DogfoodNetAnalyzersVersion)" />

The current version is
<DogfoodNetAnalyzersVersion>5.0.4-preview1.21126.5</DogfoodNetAnalyzersVersion>

So you can use/copy DLLs from artifacts\bin\Microsoft.CodeAnalysis.CSharp.NetAnalyzers\Debug\netstandard2.0 and replace with the ones in %USERPROFILE%\.nuget\packages\Microsoft.CodeAnalysis.NetAnalyzers\%RUNTIMEPACKAGEVERSION%\analyzers\dotnet\cs (just like testing the runtime repo)

@fowl2
Copy link
Contributor Author

fowl2 commented Mar 11, 2022

done:

  • rebased on main: still builds ✅🎉
  • got it running against roslyn-analyzers: no problems found
  • double checked by adding a problem: found 🎉

to be:

  • check against runtime (ran into some python dep issue) and roslyn (unrelated laptop death)

@fowl2
Copy link
Contributor Author

fowl2 commented Jan 11, 2023

I've rebased to main, changed to new diagnostic ID (again) and updated to the new DiagnosticDescriptor/LocalizableString pattern.

Ran out of time/cpu again trying to run it against runtime and rosyln, could I ask someone else have a go to help get this over the line? :)

Copy link
Contributor

@buyaa-n buyaa-n left a comment

Choose a reason for hiding this comment

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

Ran out of time/cpu again trying to run it against runtime and rosyln, could I ask someone else have a go to help get this over the line? :)

Tested with runtime looks good and no diagnostic found, thank you. We can skip testing with roslyn.

Overall looks great to me, left a few questions

@buyaa-n
Copy link
Contributor

buyaa-n commented Jan 17, 2023

Thank you @fowl2 LGTM!

@mavasani please look at the open questions and you also have out-dated change request

@buyaa-n buyaa-n merged commit 9b5d130 into dotnet:main Jan 25, 2023
@github-actions github-actions bot added this to the vNext milestone Jan 25, 2023
@fowl2 fowl2 deleted the EnumerableCasts branch January 25, 2023 01:49
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.

10 participants