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

SymbolStartAnalysisContext: Add support for VB #9077

Closed
martin-strecker-sonarsource opened this issue Apr 11, 2024 · 2 comments · Fixed by #9319
Closed

SymbolStartAnalysisContext: Add support for VB #9077

martin-strecker-sonarsource opened this issue Apr 11, 2024 · 2 comments · Fixed by #9319
Assignees
Labels
Area: VB.NET VB.NET rules related issues.

Comments

@martin-strecker-sonarsource
Copy link
Contributor

martin-strecker-sonarsource commented Apr 11, 2024

#8799 introduced the SymbolStartContextWrapper and also added a dependency to Microsoft.CodeAnalysis.VisualBasic.Workspaces to the CFG project. Subsequently issues like #9022 appeared on Peach and for customers. As a quick fix, we removed the dependency to Microsoft.CodeAnalysis.VisualBasic.Workspaces in #9027 effectively dropping VB support in RegisterCodeBlockStartAction and RegisterSyntaxNodeAction.
We should re-enable support for VB for both methods.

#9028 is a stub-implementation on how such support can be implemented without relying on Microsoft.CodeAnalysis.VisualBasic.Workspaces. Another approach would be to cache <string, Delegate> instances of dynamically typed lambdas which can be Delegate.DynamicInvoke. This would also involve MethodInfo.MakeGenericMethod and returning a compiled Delegate (because it would be open generic) instead of Action<...> from the Linq.Expression.

@pavel-mikula-sonarsource
Copy link
Contributor

Could this be solved by code duplication? To create the necessary class in VB analyzer and let the magic that requires VB reference happen only in that class?

@martin-strecker-sonarsource
Copy link
Contributor Author

That would break the API I'm afraid. The api is RegisterSyntaxKind<TLanguageKindEnum> where TLanguageKindEnum is either VB.SyntaxKind or CS.SyntaxKind. If we move the implementation, the whole wrapper gets split and the idea to be able to move to a new Rsolyn version, remove the wrapper and all work as before would be broken.
#9028 is much better approach and #9077 mentions other options as well. In the worst case, we just go dynamic which should also work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: VB.NET VB.NET rules related issues.
Projects
None yet
2 participants