Add analyzer/fixer that notifies people about explicit (but hidden) casts the compiler generates for foreach-statements. #60120

CyrusNajmabadi · 2022-03-11T20:27:43Z

This is gated behind a codestyle option called:

dotnet_style_prefer_foreach_explicit_cast_in_source

It has three potential values:

always: the cast must always be explicitly written in source if this casting was actually intentional.
never: it's fine to not have the cast be explicitly written in source.
non_legacy (default): for modern APIs the cast must be in source. for legacy APIs it's acceptable for ergonomic reasons to leave the cast off. A legacy API is one that doesn't implement IEnumerable<T> and returns object values. These are pre-2.0 APIs in the .net framework that have never been updated. The language feature here of implicitly emitted explicit casts was intentionally created to make using these APIs not as painful.

In general, for modern APIs, having this explicit cast be emitted is likely a bug indicating the user used the wrong foreach-type which will fail at runtime, but the compiler allows at compile time because it cannot prove failure. The user should either switch to a known safe (implicit) conversion which will not have any risk at runtime, or they should use an explicit cast int eh code to make it clear waht's going to happen.

A fixer comes with this that will insert that explicit cast for the user.

Tests graciously provided by @maxkoshevoi! Thank you very much. I didn't realize you already did the fixer too when i started on this on our end :'(

CyrusNajmabadi · 2022-03-11T20:28:11Z

src/Analyzers/CSharp/Analyzers/ForEachCast/CSharpForEachCastDiagnosticAnalyzer.cs

+namespace Microsoft.CodeAnalysis.CSharp.Analyzers.ForEachCast
+{
+    [DiagnosticAnalyzer(LanguageNames.CSharp)]
+    internal sealed class CSharpForEachCastDiagnosticAnalyzer : AbstractForEachCastDiagnosticAnalyzer<


i've only done the C# side for now. but the VB side wuold be trivial for someone to add as well.

src/Analyzers/Core/CodeFixes/ForEachCast/AbstractForEachCastCodeFixProvider.cs

…deFixProvider.cs

maxkoshevoi · 2022-03-11T20:35:59Z

Hm, yes, original PR links got broken when issue moved around.

So, should CA2259: Use implicitly assignable foreach variable type roslyn-analyzers#4868 be closed? It was reviewed and ready to go a year ago (but your configurable three value solution is better)
Apart from the tests, diagnostic message wording might be useful for this PR as well
General question. New analyzers are merged into roslyn repo now? Is roslyn-analyzers still used for something?

Youssef1313 · 2022-03-11T20:36:55Z

src/Analyzers/Core/Analyzers/ForEachCast/AbstractForEachCastDiagnosticAnalyzer.cs

+            context.ReportDiagnostic(DiagnosticHelper.Create(
+                Descriptor,
+                node.GetFirstToken().GetLocation(),
+                Descriptor.GetEffectiveSeverity(options),


I recall @mavasani mentioned that option.Notification.Severity should be used to support the option = value:severity syntax. Not sure if that applies here.

… into forEachPrefs

Youssef1313 · 2022-03-11T20:38:54Z

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/CodeStyle/CodeStyleOptions2.cs

+                KeyValuePairUtil.Create("non_legacy", ForEachExplicitCastInSourcePreference.NonLegacy),
+            });
+
+        internal static readonly PerLanguageOption2<CodeStyleOption2<ForEachExplicitCastInSourcePreference>> ForEachExplicitCastInSource =


AutomationObject may need an update.

enh... but does it really?

@CyrusNajmabadi It affects importing and exporting VS settings. I recall @jmarolf added some missing options few years ago, so he might have a better idea. Usually I find myself adding options there.

#16512 That was the PR.

I added an option the other day. I added code to AutomationObject in the same PR. I missed seeing you comment on my PR @Youssef1313 😁

Yes, if the expectation is that this option is round-trip-able in Visual Studio then support for it needs to be in the AutomationObject since that is what allows us to serialize VS Settings xml

i wasn't intending to add a VS option for this. This seemed like a very niche thing to have a user visible option on. I think this is more for users who run into this issue and decide to opt in at the editorconfig level. But we can def discuss this on monday if people feel differently :)

@CyrusNajmabadi In that case, I wonder why there is a RoamingProfileStorageLocation for this option? Shouldn't EditorConfigStorageLocation alone be sufficient?

yup. that makes sense. will fixup. thanks! :)

Youssef1313 · 2022-03-11T20:40:15Z

...orkspaces/SharedUtilitiesAndExtensions/Compiler/Core/CodeStyle/UnusedParametersPreference.cs

@@ -2,6 +2,8 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.

+using System.Collections.Generic;


This is the only change in this file.

RikkiGibson · 2022-03-11T20:41:34Z

Sorry, I know I just asked this offline, but do code style analyzers run during command line builds (say, when building with a recent .NET SDK)? I thought they do not.

Youssef1313 · 2022-03-11T20:48:37Z

Sorry, I know I just asked this offline, but do code style analyzers run during command line builds (say, when building with a recent .NET SDK)? I thought they do not.

They don't by default, but can be opted-in using EnforceCodeStyleInBuild

...orkspaces/SharedUtilitiesAndExtensions/Compiler/Core/CodeStyle/UnusedParametersPreference.cs

…Style/UnusedParametersPreference.cs

davidwengier · 2022-03-11T20:53:58Z

Apologies if this is not the right time, but this is the first I've seen of this idea, and it strikes me as a little odd. An analyzer that says "this could throw at runtime" is great, but its strange to me that the "fix" changes the code to something that could still throw at runtime in exactly the same way.

Was there any alternatives suggested? Like also a fix to OfType (though granted it's a little "on error resume next"-y and definitely changes semantics)? Or fix to var and leave the semantic changing as an exercise for the user?

Apologies if this went through a design meeting I missed, I'm on my phone and have read all of the backstory.

Youssef1313 · 2022-03-11T20:56:14Z

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/CodeStyle/CodeStyleOptions2.cs

+            CodeStyleOption2<ForEachExplicitCastInSourcePreference> option,
+            CodeStyleOption2<ForEachExplicitCastInSourcePreference> defaultValue)
+        {
+            Debug.Assert(s_forEachExplicitCastInSourcePreferencePreferenceMap.ContainsValue(option.Value));


What if the user provided an invalid value in editorconfig?

we debug assert :) but it doesn't hit anyone at runtime. this helps us capture issues in internal debug builds that are more likely to be a code issue. but in release this doesn't happen since clearly it could happen with a user.

CyrusNajmabadi · 2022-03-11T21:00:14Z

General question. New analyzers are merged into roslyn repo now? Is roslyn-analyzers still used for something?

It depends. If we feel this has a useful style/fix, then we are more ok to put here.

…refs

JoeRobich · 2022-03-21T21:34:35Z

Design Notes 2022/03/21

Discussion

This is implemented as a CodeStyle analyzer, but feel more like CodeQuality. Is this something people would want failing their build?
- This does feel like style and has a fixer that makes the implicit cast an explicit cast.
Don't like the non-legacy option value. Maybe something like strongly-typed or generic.
Potentially can drop the always option.

Proposal
Remove always option and rename the non-legacy option.

…refs

CyrusNajmabadi · 2022-03-22T16:45:59Z

This is ready for review after the last design pass.

src/Analyzers/Core/Analyzers/EnforceOnBuildValues.cs

Youssef1313 · 2022-03-22T16:56:17Z

src/EditorFeatures/CSharpTest/ForEachCast/ForEachCastTests.cs

+        void Main(object[] x)
+        {
+            [|foreach|] (string item in x)


I'm curious about testing tuples

void Main((object, object)[] x) { foreach ((string, string) item in x) { } }

This is interesting due to:

using System; using System.Linq; Program.M(new (object, object)[] { ("AB", "CD") }); public static class Program { public static void M((object, object)[] x) { foreach ((string, string) item in x //.Cast<(string, string)>() // Uncomment this. Crash!! ) { Console.WriteLine(item.Item1); Console.WriteLine(item.Item2); } } }

tuple tests are at the bottom of teh file.

@CyrusNajmabadi I found them, but none seems to match the above case.

//.Cast<(string, string)>() // Uncomment this. Crash!!

Added test to show we do the right thing here (no crash) :)

src/VisualStudio/CSharp/Test/EditorConfigSettings/DataProvider/DataProviderTests.cs

CyrusNajmabadi · 2022-03-22T18:19:00Z

/azp run

azure-pipelines · 2022-03-22T18:25:40Z

Azure Pipelines successfully started running 4 pipeline(s).

CyrusNajmabadi added 6 commits March 11, 2022 10:56

Analyzer and fixer for likely foreach cast issues.

3062b97

in progrss

2fbd86a

Fixes

33c4082

Fixes

76469a8

Fix tests

9b7350c

Fix tests

1c4e60f

CyrusNajmabadi requested a review from RikkiGibson March 11, 2022 20:27

CyrusNajmabadi requested a review from a team as a code owner March 11, 2022 20:27

dotnet-issue-labeler bot added the Area-IDE label Mar 11, 2022

CyrusNajmabadi commented Mar 11, 2022

View reviewed changes

CyrusNajmabadi added 3 commits March 11, 2022 12:28

Add comment

3d51fef

Add comment

7765ca8

merge ifs

ae2cabe

CyrusNajmabadi commented Mar 11, 2022

View reviewed changes

src/Analyzers/Core/CodeFixes/ForEachCast/AbstractForEachCastCodeFixProvider.cs Show resolved Hide resolved

Update src/Analyzers/Core/CodeFixes/ForEachCast/AbstractForEachCastCo…

527fc07

…deFixProvider.cs

Youssef1313 reviewed Mar 11, 2022

View reviewed changes

CyrusNajmabadi added 2 commits March 11, 2022 12:37

Unify

fc5047b

Merge branch 'forEachPrefs' of https://github.com/CyrusNajmabadi/roslyn…

3e37f11

… into forEachPrefs

Youssef1313 reviewed Mar 11, 2022

View reviewed changes

Use the right severity

06bc21d

RikkiGibson self-assigned this Mar 11, 2022

Youssef1313 reviewed Mar 11, 2022

View reviewed changes

CyrusNajmabadi commented Mar 11, 2022

View reviewed changes

...orkspaces/SharedUtilitiesAndExtensions/Compiler/Core/CodeStyle/UnusedParametersPreference.cs Outdated Show resolved Hide resolved

Update src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Code…

3009e1e

…Style/UnusedParametersPreference.cs

Youssef1313 reviewed Mar 11, 2022

View reviewed changes

CyrusNajmabadi added 2 commits March 21, 2022 10:56

Merge remote-tracking branch 'upstream/release/dev17.3' into forEachP…

7aa7734

…refs

Simplify

908505f

CyrusNajmabadi added 4 commits March 22, 2022 09:17

Merge remote-tracking branch 'upstream/release/dev17.3' into forEachP…

a877cf6

…refs

review feedback

d1e4497

Fix tst

a034bda

fixup tests

59f32c5

Youssef1313 reviewed Mar 22, 2022

View reviewed changes

src/Analyzers/Core/Analyzers/EnforceOnBuildValues.cs Outdated Show resolved Hide resolved

Youssef1313 reviewed Mar 22, 2022

View reviewed changes

CyrusNajmabadi added 2 commits March 22, 2022 10:09

move

e4031f1

dd test

eb820cc

Youssef1313 reviewed Mar 22, 2022

View reviewed changes

src/VisualStudio/CSharp/Test/EditorConfigSettings/DataProvider/DataProviderTests.cs Show resolved Hide resolved

update comment

1dd18b3

This was referenced Mar 22, 2022

InitializeToolset flakiness in correctness builds #58199

Closed

Failure in CSharpCodeActions.GFUFuzzyMatchAfterRenameTrackingAndAfterGenerateType #57423

Closed

CyrusNajmabadi mentioned this pull request Mar 23, 2022

Champion "readonly for locals and parameters" dotnet/csharplang#188

Closed

5 tasks

CyrusNajmabadi merged commit 058889b into dotnet:release/dev17.3 Mar 23, 2022

CyrusNajmabadi deleted the forEachPrefs branch March 23, 2022 22:17

maxkoshevoi mentioned this pull request Mar 23, 2022

CA2259: Use implicitly assignable foreach variable type dotnet/roslyn-analyzers#4868

Closed

jinujoseph added the Feature Request label Mar 24, 2022

binki mentioned this pull request Aug 9, 2022

Document the alternative available in visualstudio-17.3+ binki/OhNoPub.ImplicitCastAnalyzer#2

Open

cremor mentioned this pull request Aug 16, 2022

Document new EditorConfig style rules dotnet/docs#28791

Closed

10 tasks

RehanSaeed mentioned this pull request Aug 16, 2022

New Settings in VS 16.11 RehanSaeed/EditorConfig#60

Open

twest820 mentioned this pull request Aug 19, 2022

IDE0220 on foreach (Match m in regex.Matches) – Should Not Occur #63470

Closed

This was referenced Dec 12, 2022

At compile type, foreach is not validating strongly type of the elements dotnet/roslyn-analyzers#5906

Closed

[Proposal] Warn/error when foreach iteration variable is explicitly typed differently from the T in the IEnumerable<T> dotnet/roslyn-analyzers#1494

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add analyzer/fixer that notifies people about explicit (but hidden) casts the compiler generates for foreach-statements. #60120

Add analyzer/fixer that notifies people about explicit (but hidden) casts the compiler generates for foreach-statements. #60120

CyrusNajmabadi commented Mar 11, 2022

CyrusNajmabadi Mar 11, 2022

maxkoshevoi commented Mar 11, 2022 •

edited

Loading

Youssef1313 Mar 11, 2022

CyrusNajmabadi Mar 11, 2022

Youssef1313 Mar 11, 2022

CyrusNajmabadi Mar 11, 2022

Youssef1313 Mar 11, 2022

Youssef1313 Mar 11, 2022

davidwengier Mar 11, 2022

jmarolf Mar 11, 2022

CyrusNajmabadi Mar 12, 2022

Youssef1313 Mar 12, 2022 •

edited

Loading

CyrusNajmabadi Mar 12, 2022

Youssef1313 Mar 11, 2022

RikkiGibson commented Mar 11, 2022

Youssef1313 commented Mar 11, 2022

davidwengier commented Mar 11, 2022

Youssef1313 Mar 11, 2022

CyrusNajmabadi Mar 11, 2022

CyrusNajmabadi commented Mar 11, 2022

JoeRobich commented Mar 21, 2022

CyrusNajmabadi commented Mar 22, 2022

Youssef1313 Mar 22, 2022

Youssef1313 Mar 22, 2022

CyrusNajmabadi Mar 22, 2022

Youssef1313 Mar 22, 2022

CyrusNajmabadi Mar 22, 2022

CyrusNajmabadi commented Mar 22, 2022

azure-pipelines bot commented Mar 22, 2022

Add analyzer/fixer that notifies people about explicit (but hidden) casts the compiler generates for foreach-statements. #60120

Add analyzer/fixer that notifies people about explicit (but hidden) casts the compiler generates for foreach-statements. #60120

Conversation

CyrusNajmabadi commented Mar 11, 2022

Choose a reason for hiding this comment

maxkoshevoi commented Mar 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Youssef1313 Mar 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson commented Mar 11, 2022

Youssef1313 commented Mar 11, 2022

davidwengier commented Mar 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Mar 11, 2022

JoeRobich commented Mar 21, 2022

Design Notes 2022/03/21

CyrusNajmabadi commented Mar 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Mar 22, 2022

azure-pipelines bot commented Mar 22, 2022

maxkoshevoi commented Mar 11, 2022 •

edited

Loading

Youssef1313 Mar 12, 2022 •

edited

Loading