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.

Sign up for GitHub

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

Jump to bottom

Simplifier options #60174

Merged
merged 14 commits into from
Apr 14, 2022
Merged

Simplifier options #60174

merged 14 commits into from
Apr 14, 2022

Conversation

tmat
Copy link
Member

@tmat tmat commented Mar 14, 2022 •
edited
Loading

Add (CSharp|VisualBasic)SimplifierOptions containing all code style options used by simplifiers.

The first commit introduces SimplifierOptions and threads them through.

Since these options are editorconfig options we also need to pass around "fallback options" that are used when a simplifier option is not specified in the editorconfig files applicable to the given document. The fallback options are only available in the IDE - they are stored in the registry and accessed via IGlobalOptionService. In this PR fallback options are not required since we still fall back from DocumentOptions to the solution options. To limit the size of the PR some call sites that create options pass null fallback options -- these will be updated in a follow up PR.

Since the set of available simplifier options is language specific, we can't use a single SimplifierOptionsStorage extension method defined for IGlobalOptionService as it does not know how to construct the derived option types. We introduce ISimplifierOptionStorage language service for that purpose.

Recommended to review commit by commit. The first commit already reviewed by @ryzngard.

@tmat tmat force-pushed the SimplifierOptions branch 2 times, most recently from 2047746 to a930435 Compare March 16, 2022 00:37
@tmat tmat marked this pull request as ready for review March 16, 2022 00:37
@tmat tmat requested a review from a team as a code owner March 16, 2022 00:37
ryzngard
ryzngard approved these changes Mar 16, 2022
View reviewed changes
Copy link
Contributor

@ryzngard ryzngard left a comment

Choose a reason for hiding this comment

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

:shipit:

src/Workspaces/VisualBasicTest/CodeGeneration/AddImportsTests.vb Outdated Show resolved Hide resolved
@tmat tmat marked this pull request as draft March 16, 2022 14:35
This was referenced Mar 18, 2022
Closed
Open
@sharwell
Copy link
Member

sharwell commented Apr 5, 2022

Integration tests showing gold bar

Feature 'Diagnostic analyzer runner' is currently unavailable due to an internal error.
MessagePack.MessagePackSerializationException : Error writing JSON RPC Message: MessagePackSerializationException: Failed to serialize Microsoft.CodeAnalysis.Diagnostics.DiagnosticArguments value. ---> MessagePack.MessagePackSerializationException : Failed to serialize Microsoft.CodeAnalysis.Diagnostics.DiagnosticArguments value. ---> MessagePack.FormatterNotRegisteredException : Microsoft.CodeAnalysis.Simplification.SimplifierOptions is not registered in resolver: StreamJsonRpc.MessagePackFormatter+ResolverWrapper
   at MessagePack.FormatterResolverExtensions.Throw(Type t,IFormatterResolver resolver)
   at MessagePack.FormatterResolverExtensions.GetFormatterWithVerify[T](IFormatterResolver resolver)
   at Serialize(Byte[][] ,Object[] ,MessagePackWriter& ,IdeAnalyzerOptions ,MessagePackSerializerOptions )
   at MessagePack.Internal.AnonymousSerializableFormatter`1.Serialize(MessagePackWriter& writer,T value,MessagePackSerializerOptions options)
   at Serialize(Byte[][] ,Object[] ,MessagePackWriter& ,DiagnosticArguments ,MessagePackSerializerOptions )
   at MessagePack.Internal.AnonymousSerializableFormatter`1.Serialize(MessagePackWriter& writer,T value,MessagePackSerializerOptions options)
   at MessagePack.MessagePackSerializer.Serialize[T](MessagePackWriter& writer,T value,MessagePackSerializerOptions options)
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at Microsoft.VisualStudio.Telemetry.WindowsErrorReporting.WatsonReport.GetClrWatsonExceptionInfo(Exception exceptionObject)
   --- End of inner exception stack trace ---
   at MessagePack.MessagePackSerializer.Serialize[T](MessagePackWriter& writer,T value,MessagePackSerializerOptions options)
   at StreamJsonRpc.MessagePackFormatter.JsonRpcRequestFormatter.Serialize(MessagePackWriter& writer,JsonRpcRequest value,MessagePackSerializerOptions options)
   at StreamJsonRpc.MessagePackFormatter.JsonRpcMessageFormatter.Serialize(MessagePackWriter& writer,JsonRpcMessage value,MessagePackSerializerOptions options)
   at StreamJsonRpc.MessagePackFormatter.Serialize(IBufferWriter`1 contentBuffer,JsonRpcMessage message)
   --- End of inner exception stack trace ---
   at StreamJsonRpc.MessagePackFormatter.Serialize(IBufferWriter`1 contentBuffer,JsonRpcMessage message)
   at StreamJsonRpc.LengthHeaderMessageHandler.Write(JsonRpcMessage content,CancellationToken cancellationToken)
   at StreamJsonRpc.PipeMessageHandler.WriteCoreAsync(JsonRpcMessage content,CancellationToken cancellationToken)
   at async StreamJsonRpc.MessageHandlerBase.WriteAsync(<Unknown Parameters>)
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at async StreamJsonRpc.JsonRpc.SendAsync(<Unknown Parameters>)
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at async StreamJsonRpc.JsonRpc.InvokeCoreAsync(<Unknown Parameters>)
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at async StreamJsonRpc.JsonRpc.InvokeCoreAsync[TResult](<Unknown Parameters>)
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at async Microsoft.CodeAnalysis.Remote.BrokeredServiceConnection`1.TryInvokeAsync[TService,TResult](<Unknown Parameters>)

@tmat tmat force-pushed the SimplifierOptions branch 4 times, most recently from 579ac53 to 5a8b5e2 Compare April 12, 2022 04:37
@tmat tmat force-pushed the SimplifierOptions branch 5 times, most recently from 003608b to 21d8d35 Compare April 14, 2022 00:03
CyrusNajmabadi
CyrusNajmabadi reviewed Apr 14, 2022
View reviewed changes
src/EditorFeatures/Test2/Simplification/AbstractSimplificationTests.vb
Dim simplifiedDocument As Document
If Not explicitSpansToSimplifyWithin.IsDefaultOrEmpty Then
simplifiedDocument = Await Simplifier.ReduceAsync(document, explicitSpansToSimplifyWithin, optionSet)
Else
simplifiedDocument = Await Simplifier.ReduceAsync(document, Simplifier.Annotation, optionSet)
End If
#Enable Warning RS0030
Copy link
Member

Choose a reason for hiding this comment

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

could just update this to use the new apis right?

CyrusNajmabadi
CyrusNajmabadi reviewed Apr 14, 2022
View reviewed changes
src/Features/Core/Portable/MetadataAsSource/MetadataAsSourceOptions.cs
bool NavigateToDecompiledSources = true,
bool AlwaysUseDefaultSymbolServers = true)
{
public static readonly MetadataAsSourceOptions Default = new();

public MetadataAsSourceOptions()
: this(NavigateToDecompiledSources: true)
: this(SimplifierOptions: null)
Copy link
Member

Choose a reason for hiding this comment

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

aside: i'm not really a fan of using structs here primarily because i find having to reason about no-param constructors (vs, all optional constructors) so painful. Consider moving these to classes.

CyrusNajmabadi
CyrusNajmabadi reviewed Apr 14, 2022
View reviewed changes
...s/CSharp/Portable/Simplification/Reducers/AbstractCSharpReducer.AbstractReductionRewriter.cs Outdated
{
Contract.ThrowIfNull(ParseOptions);
Debug.Assert(Options != null);
Debug.Assert(SemanticModel != null);
Copy link
Member

Choose a reason for hiding this comment

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

just use Contract.ThrowIfNull for all of htem :)

@tmat tmat enabled auto-merge (squash) April 14, 2022 20:52
@tmat tmat merged commit 2b1fbd8 into dotnet:main Apr 14, 2022
@ghost ghost added this to the Next milestone Apr 14, 2022
@tmat tmat deleted the SimplifierOptions branch April 14, 2022 22:07
333fred added a commit that referenced this pull request Apr 14, 2022
@333fred
Merge remote-tracking branch 'upstream/main' into merge-main-to-featu…
16a02f8 
…res/required-members

* upstream/main: (66 commits)
  Fix #55183: Add SymbolVisitor<TArgument, TResult> (#56530)
  Simplifier options (#60174)
  Remove duplicated asset
  Do not try to refcount solution syncing when communicating with OOP
  Delay symbol-search index updating until solution is fully loaded.
  add more miscellaneous tests for checked operators (#60727)
  Support checked operators in explicit interface implementation (#60715)
  Avoid formatting diagnostics with raw strings (#60655)
  Make heading levels for warning waves documentation consistent (#60721)
  Clean up IDiagnosticService extension methods
  Remove #nullable enable
  Add integration test to flag MEF composition breaks
  Generate static abstract interface members correctly (#60618)
  Merge release/dev17.2 to main (#60682)
  Fix FAR on checked operators (#60698)
  Add implement interface support for checked operators and cast operators (#60719)
  Update csc.dll path in launch.json (#60663)
  Grab bag of UTF8 string support in IDE features (#60599)
  Allow code actions to retrieve options for any language (#60697)
  Fix flaky VSTypeScriptHandlerTests  (#60706)
  ...
333fred added a commit to 333fred/roslyn that referenced this pull request Apr 15, 2022
@333fred
Merge remote-tracking branch 'upstream/features/required-members' int…
728b6da 
…o setsrequiredmembers

* upstream/features/required-members: (66 commits)
  Fix dotnet#55183: Add SymbolVisitor<TArgument, TResult> (dotnet#56530)
  Simplifier options (dotnet#60174)
  Remove duplicated asset
  Do not try to refcount solution syncing when communicating with OOP
  Delay symbol-search index updating until solution is fully loaded.
  add more miscellaneous tests for checked operators (dotnet#60727)
  Support checked operators in explicit interface implementation (dotnet#60715)
  Avoid formatting diagnostics with raw strings (dotnet#60655)
  Make heading levels for warning waves documentation consistent (dotnet#60721)
  Clean up IDiagnosticService extension methods
  Remove #nullable enable
  Add integration test to flag MEF composition breaks
  Generate static abstract interface members correctly (dotnet#60618)
  Merge release/dev17.2 to main (dotnet#60682)
  Fix FAR on checked operators (dotnet#60698)
  Add implement interface support for checked operators and cast operators (dotnet#60719)
  Update csc.dll path in launch.json (dotnet#60663)
  Grab bag of UTF8 string support in IDE features (dotnet#60599)
  Allow code actions to retrieve options for any language (dotnet#60697)
  Fix flaky VSTypeScriptHandlerTests  (dotnet#60706)
  ...
@dibarbet dibarbet modified the milestones: Next, 17.3.P1 Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryzngard ryzngard ryzngard approved these changes

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi approved these changes

@333fred 333fred Awaiting requested review from 333fred 333fred is a code owner

@JoeRobich JoeRobich Awaiting requested review from JoeRobich

Assignees
No one assigned
Labels
Area-IDE
Projects
None yet
Milestone
17.3.P1
Development

Successfully merging this pull request may close these issues.
5 participants
@tmat @sharwell @ryzngard @CyrusNajmabadi @dibarbet