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

Fix #55183: Add SymbolVisitor<TArgument, TResult> #56530

Merged
merged 2 commits into from
Apr 14, 2022
Merged

Fix #55183: Add SymbolVisitor<TArgument, TResult> #56530

merged 2 commits into from
Apr 14, 2022

Conversation

siegfriedpammer
Copy link
Contributor

@siegfriedpammer siegfriedpammer commented Sep 19, 2021 •
edited by jcouv
Loading

Fixes #55183

@siegfriedpammer siegfriedpammer requested review from a team as code owners September 19, 2021 14:12
@ghost ghost added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Sep 19, 2021
@RikkiGibson
Copy link
Contributor

@333fred are we waiting until main becomes 17.1 to review/merge this? Or can it go into 17.0?

@333fred
Copy link
Member

333fred commented Sep 22, 2021

Not sure what the bar for 17.0 is at this point. @jaredpar?

333fred
333fred reviewed Sep 22, 2021
View reviewed changes
Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

We do need to have explicit testing of the new visitor.

@jcouv jcouv added this to the 17.1 milestone Sep 27, 2021
@jcouv
Copy link
Member

jcouv commented Sep 28, 2021

@siegfriedpammer Let us know if you need some pointers on how to test this change. Thanks

@siegfriedpammer
Copy link
Contributor Author

I tried looking for tests of the other visitors however, I could not find any samples. Any pointers would be appreciated. Thanks!

@jcouv
Copy link
Member

jcouv commented Sep 28, 2021

I would create a new test file under src\Compilers\CSharp\Test\Symbol\Compilation called SymbolVisitorTests.cs with a nested type that implements the new SymbolVisitor with simple traversal and logging (adding some text to a StringBuilder).
Then create a simple program with various kinds of symbols (types, methods, aliases, etc. We can assist if you're not sure how to reach some of the cases), make a compilation with it (CreateCompilation test helper), grab its .SourceAssembly, use the visitor on it and verify the contents of the builder.
Then something similar for VB.

Let me know if that makes sense.

@jcouv
Copy link
Member

jcouv commented Oct 11, 2021

Marking this PR as draft for now (to make room in our review queue while the PR is being worked on). Please ping if you have any questions or when ready for another look.

@jcouv jcouv marked this pull request as draft October 11, 2021 18:29
@333fred
Copy link
Member

333fred commented Nov 10, 2021

@siegfriedpammer is there anything you need from us to address feedback? Feel free to chat here, or I'm available on discord (id Orannis#3333).

@siegfriedpammer

This comment has been minimized.

Sign in to view
@siegfriedpammer

This comment has been minimized.

Sign in to view
@siegfriedpammer

This comment has been minimized.

Sign in to view
@siegfriedpammer

This comment has been minimized.

Sign in to view
@siegfriedpammer siegfriedpammer mentioned this pull request Nov 11, 2021
Closed
@siegfriedpammer siegfriedpammer force-pushed the dev/siegfriedpammer/issue55183 branch from ec223bc to 3f45587 Compare December 20, 2021 13:12
@siegfriedpammer
Copy link
Contributor Author

siegfriedpammer commented Dec 20, 2021 •
edited
Loading

@333fred I am trying to get this wrapped up. I am not sure how you would approach the topic of unit testing the visitor. I have pushed my first try and of course it is still unfinished. Would it be possible for you to take a look and provide feedback nonetheless? Thank you very much!

@333fred
Copy link
Member

333fred commented Dec 20, 2021

@siegfriedpammer that looks great. Just need some for the new API :).

@siegfriedpammer
Copy link
Contributor Author

Thanks, then I will continue and get this done!

@siegfriedpammer siegfriedpammer force-pushed the dev/siegfriedpammer/issue55183 branch from 3f45587 to 070265f Compare April 10, 2022 07:58
@siegfriedpammer siegfriedpammer marked this pull request as ready for review April 10, 2022 09:40
@siegfriedpammer siegfriedpammer requested a review from a team as a code owner April 10, 2022 09:40
333fred
333fred approved these changes Apr 11, 2022
View reviewed changes
Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

LGTM (commit 2). @dotnet/roslyn-compiler for a second review.

AlekseyTs
AlekseyTs approved these changes Apr 14, 2022
View reviewed changes
Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 2)

@AlekseyTs
Copy link
Contributor

@dotnet/roslyn-ide For a sign-off

ryzngard
ryzngard approved these changes Apr 14, 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.

IDE code LGTM

@333fred 333fred merged commit 646e3c4 into dotnet:main Apr 14, 2022
@ghost ghost modified the milestones: 17.1, Next Apr 14, 2022
@333fred
Copy link
Member

333fred commented Apr 14, 2022

Thanks for the contribution @siegfriedpammer!

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
@lameox lameox mentioned this pull request Jul 14, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlekseyTs AlekseyTs AlekseyTs approved these changes

@333fred 333fred 333fred approved these changes

@ryzngard ryzngard ryzngard approved these changes

Assignees
No one assigned
Labels
Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee.
Projects
None yet
Milestone
17.3.P1
Development

Successfully merging this pull request may close these issues.

Add SymbolVisitor<TArgument, TResult>
7 participants
@siegfriedpammer @RikkiGibson @333fred @jcouv @AlekseyTs @ryzngard @dibarbet