This should have an option

The day where this isn't needed anymore can't get here soon enough

For the option to be useful, it needs to be involved in code generation. Otherwise, setting severity should be enough. Anything I'm missing?

yeah, we need an option so that people can configure this from the UI. for example, stating that they want this, and what severity this should be.

@CyrusNajmabadi Won't @jmarolf's PR (#51069) address this in a more global way (for all analyzers)?

I'll add an option for now, but may jump into discord to get more information.

In the analyzer you check for Object creation. Here you cast to New expression. (I'm not at computer), is this safe?

@CyrusNajmabadi Nice catch! I think the cast may fail. Will come up with a test and fix that.

The cast doesn't fail since an ObjectCreationExpressionSyntax derives from NewExpressionSyntax (i.e, it's always safe to cast an ObjectCreationExpressionSyntax to its base type. However, the analyzer is missing cases.

@CyrusNajmabadi The syntactic simplification of an expression like Dim result As C() = New C() { } (i.e, ArrayCreationExpression) will be different since Dim result As New C() { } isn't allowed by the language. Let me know if you want me to handle this or keep it simple and only catch ObjectCreationExpressionSyntax.

just keep it simple. make sure we have tests and whatnot.

we would want an editorconfig value for this. otherwise people can't disable the hint. or, alternatively, some projects might want to make it an error if you use the old form and not the new form.

i think this loses the value of hte style option. i.e. the severity.

@CyrusNajmabadi Can you elaborate more on this?

i think this loses the value of hte style option. i.e. the severity.

Aren't these actively being deprecated? The form used in this PR does account for the standard dotnet_diagnostic.*.severity option.

Could you clarify how that works? How did the diagnostic know what severity the user has set this particular option to?

I think this happens around the following, not sure enough:

https://github.com/dotnet/roslyn/blob/1cca63b5d8ea170f8d8e88e1574aa3ebe354c23b/src/Compilers/Core/Portable/Diagnostic/DiagnosticDescriptor.cs#L213-L224

I do not understand why some of these anlayzers are using this form, and some are using DiagnosticHelper.Create. Can you please use DiagnosticHelper.Create to be consistent with the rest of the analyzers in this domain. If that approach is incorrect we cna fix them all up together.

@sharwell @mavasani to advise. Note: this is one of the reasons i really dislike making pattern changes in a piecemeal fashion. I cannot go and look and see what is the right way to do something because we aren't consistent in our own codebase.

@mavasani can you verify that all the above is correct? That it's fine for an anlyzer to just create the Diagnostic and not pass the severity. If so, i'm fine with this piece.

We have 2 core scenarios here to consider, let me elaborate:

1. Command line builds - This scenario will have no impact on whether we use DiagnosticHelper.Create(..., Notification.Severity, ...) or Diagnostic.Create(...), with the latter ignoring Notification.Severity from the code style option.
2. IDE background analysis - This scenario will have different user experience based on the chosen approach. Using DiagnosticHelper.Create(..., Notification.Severity, ...) would ensure the option severity set in Tools/Option or via the :severity syntax in editorconfig will be respected. Using Diagnostic.Create(...) would lead to this severity setting being ignored even in the IDE. We normally use Diagnostic.Create(...) for reporting analyzer diagnostics only when the IDE analyzer does not have corresponding code style option where :severity can be specified.

Overall it proves the point that we were just discussing at the design meeting - determining the reported severity of diagnostics should not be determined by the analyzers, but instead better for analyzer driver to do the right thing. I think it is fine to leave it in Dev16 and we can hopefully fix the core issue properly in the compiler/analyzer driver layer in Dev17 so users and analyzer authors have the minimal overhead for getting the most intuitive functionality.

I see. This code is incorrect then. @Youssef1313 can you revise this to address this issue?

Overall it proves the point that we were just discussing at the design meeting - determining the reported severity of diagnostics should not be determined by the analyzers, but instead better for analyzer driver to do the right thing.

I'm fine with that. My general point is that that should still be done in a way taht is palatable to the person who owns the editorconfig file. :)

pass title for equivalence key?

@CyrusNajmabadi It was suggested by @sharwell to pass NameOf(resource) as equivalence key. I don't know if it matters.

Non-localized equivalence keys may make it easier to use equivalence keys in the future for configuring/disabling specific code actions.

that won't seem to work anyways as we use the title in tons of code actions today. i woudl rather us just be consistent everywhere. if and when we have a need to make changes for future work, we can then change everything togehter. Being inconsistent here is worse, to me, because it means no one can know for certain what is going on and any particular PR has to deal with trying to rationalize what pattern should be used where.

@CyrusNajmabadi The situation is already inconsistent unfortunately. There exist usage of both patterns already 😕
So I think we should go to whatever helps in the future. We can also create an analyzer that makes sure equivalence keys are compile-time constants.

is there a fix-all test?

@CyrusNajmabadi See SimplifyObjectCreation_MultipleDeclarators.