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

Add SymbolVisitor<TArgument, TResult> #55183

Closed
siegfriedpammer opened this issue Jul 28, 2021 · 6 comments · Fixed by #56530
Closed

Add SymbolVisitor<TArgument, TResult> #55183

siegfriedpammer opened this issue Jul 28, 2021 · 6 comments · Fixed by #56530
Assignees
Labels
api-approved API was approved in API review, it can be implemented Area-Compilers Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature Request help wanted The issue is "up for grabs" - add a comment if you are interested in working on it

Comments

@siegfriedpammer
Copy link
Contributor

siegfriedpammer commented Jul 28, 2021

Background and Motivation

In order to avoid shared state and allowing parallel invocations of the same visitor, it would be nice to be able to use a SymbolVisitor<TArgument, TResult>. We already have a similar pattern with OperationVisitor.

Proposed API

namespace Microsoft.CodeAnalysis
{
+    public abstract class SymbolVisitor<TArgument, TResult>
+    {
+        public virtual TResult? Visit(ISymbol? symbol, TArgument argument)
+        {
+            if (symbol != null)
+            {
+                return symbol.Accept(this, argument);
+            }
+
+            return default(TResult);
+        }
+
+        public virtual TResult? DefaultVisit(ISymbol symbol, TArgument argument)
+        {
+            return default(TResult);
+        }
+
+        public virtual TResult? VisitAlias(IAliasSymbol symbol, TArgument argument)
+        {
+            return DefaultVisit(symbol, argument);
+        }
+
+        public virtual TResult? VisitArrayType(IArrayTypeSymbol symbol, TArgument argument)
+        {
+            return DefaultVisit(symbol, argument);
+        }
+
+        public virtual TResult? VisitAssembly(IAssemblySymbol symbol, TArgument argument)
+        {
+            return DefaultVisit(symbol, argument);
+        }
+
+        public virtual TResult? VisitDiscard(IDiscardSymbol symbol, TArgument argument)
+        {
+            return DefaultVisit(symbol, argument);
+        }
+
+        public virtual TResult? VisitDynamicType(IDynamicTypeSymbol symbol, TArgument argument)
+        {
+            return DefaultVisit(symbol, argument);
+        }
+
+        public virtual TResult? VisitEvent(IEventSymbol symbol, TArgument argument)
+        {
+            return DefaultVisit(symbol, argument);
+        }
+
+        public virtual TResult? VisitField(IFieldSymbol symbol, TArgument argument)
+        {
+            return DefaultVisit(symbol, argument);
+        }
+
+        public virtual TResult? VisitLabel(ILabelSymbol symbol, TArgument argument)
+        {
+            return DefaultVisit(symbol, argument);
+        }
+
+        public virtual TResult? VisitLocal(ILocalSymbol symbol, TArgument argument)
+        {
+            return DefaultVisit(symbol, argument);
+        }
+
+        public virtual TResult? VisitMethod(IMethodSymbol symbol, TArgument argument)
+        {
+            return DefaultVisit(symbol, argument);
+        }
+
+        public virtual TResult? VisitModule(IModuleSymbol symbol, TArgument argument)
+        {
+            return DefaultVisit(symbol, argument);
+        }
+
+        public virtual TResult? VisitNamedType(INamedTypeSymbol symbol, TArgument argument)
+        {
+            return DefaultVisit(symbol, argument);
+        }
+
+        public virtual TResult? VisitNamespace(INamespaceSymbol symbol, TArgument argument)
+        {
+            return DefaultVisit(symbol, argument);
+        }
+
+        public virtual TResult? VisitParameter(IParameterSymbol symbol, TArgument argument)
+        {
+            return DefaultVisit(symbol, argument);
+        }
+
+        public virtual TResult? VisitPointerType(IPointerTypeSymbol symbol, TArgument argument)
+        {
+            return DefaultVisit(symbol, argument);
+        }
+
+        public virtual TResult? VisitFunctionPointerType(IFunctionPointerTypeSymbol symbol, TArgument argument)
+        {
+            return DefaultVisit(symbol, argument);
+        }
+
+        public virtual TResult? VisitProperty(IPropertySymbol symbol, TArgument argument)
+        {
+            return DefaultVisit(symbol, argument);
+        }
+
+        public virtual TResult? VisitRangeVariable(IRangeVariableSymbol symbol, TArgument argument)
+        {
+            return DefaultVisit(symbol, argument);
+        }
+
+        public virtual TResult? VisitTypeParameter(ITypeParameterSymbol symbol, TArgument argument)
+        {
+            return DefaultVisit(symbol, argument);
+        }
+    }

	public interface ISymbol : IEquatable<ISymbol?>
    {
		// ...
        void Accept(SymbolVisitor visitor);

        TResult? Accept<TResult>(SymbolVisitor<TResult> visitor);

+        TResult? Accept<TArgument, TResult>(SymbolVisitor<TArgument, TResult> visitor, TArgument argument);
		// ...
    }
}

Usage Examples

      var result = symbol.Accept(new GenerateIdVisitor(), new GeneratorContext());

Alternative Designs

It would be possible to implement a visitor based on dynamic invocation, however, I think this is not the optimal solution.

Risks

none I can think of

@siegfriedpammer siegfriedpammer added Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature Request labels Jul 28, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jul 28, 2021
@jaredpar jaredpar removed the untriaged Issues and PRs which have not yet been triaged by a lead label Aug 2, 2021
@jaredpar jaredpar added this to the 17.0 milestone Aug 2, 2021
@333fred
Copy link
Member

333fred commented Aug 2, 2021

Seems reasonable. The only change I think we need to make is remove the ? from TArgument: the user should decide whether the argument is allowed to be nullable or not. @siegfriedpammer let me know when that's been updated and I can tag this for review.

@333fred 333fred added the api-needs-work API needs work before it is approved, it is NOT ready for implementation label Aug 2, 2021
@siegfriedpammer
Copy link
Contributor Author

Done. Yeah, I am pretty new to NRTs, so I had a small misconception regarding nullability of unconstrained generics, thanks for pointing that out.

Thank you very much for considering this!

@333fred 333fred added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-needs-work API needs work before it is approved, it is NOT ready for implementation labels Aug 2, 2021
@333fred
Copy link
Member

333fred commented Aug 2, 2021

We'll review this during the next review session, which would normally be next week, but as I'll be out then it will be in about 3 weeks.

@333fred 333fred modified the milestones: 17.0, 17.1 Sep 2, 2021
@333fred
Copy link
Member

333fred commented Sep 9, 2021

API Review

We slightly changed the shape of the API to allow for nullability to play better with the visitor result generic. This is inconsistent with the rest of our visitors, but we can look at taking a breaking change in a future API version to allow them to better-express the TResult nullability as well. The amended shape is as follows:

namespace Microsoft.CodeAnalysis
{
+    public abstract class SymbolVisitor<TArgument, TResult>
+    {
+        protected abstract TResult DefaultResult { get; }
+
+        public virtual TResult Visit(ISymbol? symbol, TArgument argument)
+        {
+            if (symbol != null)
+            {
+                return symbol.Accept(this, argument);
+            }
+
+            return DefaultResult;
+        }
+
+        public virtual TResult DefaultVisit(ISymbol symbol, TArgument argument)
+        {
+            return DefaultResult;
+        }
+
+        public virtual TResult VisitAlias(IAliasSymbol symbol, TArgument argument)
+        {
+            return DefaultVisit(symbol, argument);
+        }
+
+        public virtual TResult VisitArrayType(IArrayTypeSymbol symbol, TArgument argument)
+        {
+            return DefaultVisit(symbol, argument);
+        }
+
+        public virtual TResult VisitAssembly(IAssemblySymbol symbol, TArgument argument)
+        {
+            return DefaultVisit(symbol, argument);
+        }
+
+        public virtual TResult VisitDiscard(IDiscardSymbol symbol, TArgument argument)
+        {
+            return DefaultVisit(symbol, argument);
+        }
+
+        public virtual TResult VisitDynamicType(IDynamicTypeSymbol symbol, TArgument argument)
+        {
+            return DefaultVisit(symbol, argument);
+        }
+
+        public virtual TResult VisitEvent(IEventSymbol symbol, TArgument argument)
+        {
+            return DefaultVisit(symbol, argument);
+        }
+
+        public virtual TResult VisitField(IFieldSymbol symbol, TArgument argument)
+        {
+            return DefaultVisit(symbol, argument);
+        }
+
+        public virtual TResult VisitLabel(ILabelSymbol symbol, TArgument argument)
+        {
+            return DefaultVisit(symbol, argument);
+        }
+
+        public virtual TResult VisitLocal(ILocalSymbol symbol, TArgument argument)
+        {
+            return DefaultVisit(symbol, argument);
+        }
+
+        public virtual TResult VisitMethod(IMethodSymbol symbol, TArgument argument)
+        {
+            return DefaultVisit(symbol, argument);
+        }
+
+        public virtual TResult VisitModule(IModuleSymbol symbol, TArgument argument)
+        {
+            return DefaultVisit(symbol, argument);
+        }
+
+        public virtual TResult VisitNamedType(INamedTypeSymbol symbol, TArgument argument)
+        {
+            return DefaultVisit(symbol, argument);
+        }
+
+        public virtual TResult VisitNamespace(INamespaceSymbol symbol, TArgument argument)
+        {
+            return DefaultVisit(symbol, argument);
+        }
+
+        public virtual TResult VisitParameter(IParameterSymbol symbol, TArgument argument)
+        {
+            return DefaultVisit(symbol, argument);
+        }
+
+        public virtual TResult VisitPointerType(IPointerTypeSymbol symbol, TArgument argument)
+        {
+            return DefaultVisit(symbol, argument);
+        }
+
+        public virtual TResult VisitFunctionPointerType(IFunctionPointerTypeSymbol symbol, TArgument argument)
+        {
+            return DefaultVisit(symbol, argument);
+        }
+
+        public virtual TResult VisitProperty(IPropertySymbol symbol, TArgument argument)
+        {
+            return DefaultVisit(symbol, argument);
+        }
+
+        public virtual TResult VisitRangeVariable(IRangeVariableSymbol symbol, TArgument argument)
+        {
+            return DefaultVisit(symbol, argument);
+        }
+
+        public virtual TResult VisitTypeParameter(ITypeParameterSymbol symbol, TArgument argument)
+        {
+            return DefaultVisit(symbol, argument);
+        }
+    }

@333fred 333fred added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Sep 9, 2021
@333fred
Copy link
Member

333fred commented Sep 9, 2021

@siegfriedpammer sorry for the review delay, but it's now approved with the above shape. Would you be interested in implementing the API?

@333fred 333fred added the help wanted The issue is "up for grabs" - add a comment if you are interested in working on it label Sep 9, 2021
@siegfriedpammer
Copy link
Contributor Author

Yes, of course!

siegfriedpammer added a commit to siegfriedpammer/roslyn that referenced this issue Sep 19, 2021
@jcouv jcouv modified the milestones: 17.1, 17.2 Mar 17, 2022
333fred pushed a commit that referenced this issue Apr 14, 2022
* Fix #55183: Add SymbolVisitor<TArgument, TResult>
333fred added a commit that referenced this issue Apr 14, 2022
…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 issue Apr 15, 2022
…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)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented Area-Compilers Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature Request help wanted The issue is "up for grabs" - add a comment if you are interested in working on it
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants