Add analyzer/fixer that notifies people about explicit (but hidden) casts the compiler generates for foreach-statements.#60120
Conversation
| namespace Microsoft.CodeAnalysis.CSharp.Analyzers.ForEachCast | ||
| { | ||
| [DiagnosticAnalyzer(LanguageNames.CSharp)] | ||
| internal sealed class CSharpForEachCastDiagnosticAnalyzer : AbstractForEachCastDiagnosticAnalyzer< |
There was a problem hiding this comment.
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
Show resolved
Hide resolved
|
Hm, yes, original PR links got broken when issue moved around.
|
| context.ReportDiagnostic(DiagnosticHelper.Create( | ||
| Descriptor, | ||
| node.GetFirstToken().GetLocation(), | ||
| Descriptor.GetEffectiveSeverity(options), |
There was a problem hiding this comment.
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
| KeyValuePairUtil.Create("non_legacy", ForEachExplicitCastInSourcePreference.NonLegacy), | ||
| }); | ||
|
|
||
| internal static readonly PerLanguageOption2<CodeStyleOption2<ForEachExplicitCastInSourcePreference>> ForEachExplicitCastInSource = |
There was a problem hiding this comment.
AutomationObject may need an update.
There was a problem hiding this comment.
enh... but does it really?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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 😁
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
@CyrusNajmabadi In that case, I wonder why there is a RoamingProfileStorageLocation for this option? Shouldn't EditorConfigStorageLocation alone be sufficient?
There was a problem hiding this comment.
yup. that makes sense. will fixup. thanks! :)
| // 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; |
There was a problem hiding this comment.
This is the only change in this file.
|
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 |
...orkspaces/SharedUtilitiesAndExtensions/Compiler/Core/CodeStyle/UnusedParametersPreference.cs
Outdated
Show resolved
Hide resolved
…Style/UnusedParametersPreference.cs
|
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 Apologies if this went through a design meeting I missed, I'm on my phone and have read all of the backstory. |
| CodeStyleOption2<ForEachExplicitCastInSourcePreference> option, | ||
| CodeStyleOption2<ForEachExplicitCastInSourcePreference> defaultValue) | ||
| { | ||
| Debug.Assert(s_forEachExplicitCastInSourcePreferencePreferenceMap.ContainsValue(option.Value)); |
There was a problem hiding this comment.
What if the user provided an invalid value in editorconfig?
There was a problem hiding this comment.
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.
It depends. If we feel this has a useful style/fix, then we are more ok to put here. |
Design Notes 2022/03/21Discussion
Proposal |
|
This is ready for review after the last design pass. |
| void Main(object[] x) | ||
| { | ||
| [|foreach|] (string item in x) |
There was a problem hiding this comment.
I'm curious about testing tuples
void Main((object, object)[] x)
{
foreach ((string, string) item in x)
{
}
}There was a problem hiding this comment.
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);
}
}
}There was a problem hiding this comment.
tuple tests are at the bottom of teh file.
There was a problem hiding this comment.
@CyrusNajmabadi I found them, but none seems to match the above case.
There was a problem hiding this comment.
//.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
Show resolved
Hide resolved
|
/azp run |
|
Azure Pipelines successfully started running 4 pipeline(s). |
Fixes #54712
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 implementIEnumerable<T>and returnsobjectvalues. 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 :'(