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

Allow AnalyzeDataFlow for ConstructorInitializerSyntax #57577

Closed
bernd5 opened this issue Nov 4, 2021 · 20 comments · Fixed by #57576
Closed

Allow AnalyzeDataFlow for ConstructorInitializerSyntax #57577

bernd5 opened this issue Nov 4, 2021 · 20 comments · Fixed by #57576
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
Milestone

Comments

@bernd5
Copy link
Contributor

bernd5 commented Nov 4, 2021

Background and Motivation

For analyzers and rewriters it is sometimes required to analyze the data flow of a constructor initializer which is very similar to an implicit first expression statement.
Unfortenately it is not an expression nor a statement and the public API allows only to analyze statements or expressions (specified in the comment).

Proposed API

The current public API accepts all SyntaxNode but throws expections if the node is not a statement nor an expression.
Therefore we don't need to change the public API. We should just change the comment.

using System.Collections.Immutable;
using System.Threading;

namespace Microsoft.CodeAnalysis
{
    public static class ModelExtensions
    {
        ...

        /// <summary>
        /// Analyze data-flow within a part of a method body.
+       /// note (for C#): ConstructorInitializerSyntax and PrimaryConstructorBaseTypeSyntax are treated by this API as regular statements
        /// </summary>
        public static DataFlowAnalysis AnalyzeDataFlow(this SemanticModel semanticModel, SyntaxNode statementOrExpression)
        {
            //impl
        }
    }
}

Optional API extension:

To make the support for ConstructorInitializerSyntax and PrimaryConstructorBaseTypeSyntax more obvious we could add the folowing extension methods:

namespace Microsoft.CodeAnalysis
{
    public static class CSharpExtensions
    {
+        /// <summary>
+        /// Analyze data-flow within a constructor initializer.
+        /// </summary>
+        public static DataFlowAnalysis? AnalyzeDataFlow(this SemanticModel? semanticModel, ConstructorInitializerSyntax constructorInitializer)
+        {
+            //impl
+        }
+
+        /// <summary>
+        /// Analyze data-flow within a primary constructor base type.
+        /// </summary>
+        public static DataFlowAnalysis? AnalyzeDataFlow(this SemanticModel? semanticModel, PrimaryConstructorBaseTypeSyntax primaryConstructorBaseType)
+        {
+            //impl
+        }
    }
}

Usage Examples

analysis = model.AnalyzeDataFlow(ctorInit);

Alternative Designs

Some user hacked around this missing feature and created a corresponding object creation expression for the analysis which produces a quite good result. The downside is just the mapping of symbols between two different models - which can become quite complicated and error prone.

Risks

No known risk

Implementation

#57576

@bernd5 bernd5 added Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature Request labels Nov 4, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Analyzers untriaged Issues and PRs which have not yet been triaged by a lead labels Nov 4, 2021
@jinujoseph jinujoseph added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead and removed untriaged Issues and PRs which have not yet been triaged by a lead Area-Analyzers labels Nov 4, 2021
@bernd5
Copy link
Contributor Author

bernd5 commented Nov 5, 2021

@AlekseyTs @333fred: could you have a look?

@bernd5
Copy link
Contributor Author

bernd5 commented Nov 5, 2021

@jaredpar

@jaredpar jaredpar removed the untriaged Issues and PRs which have not yet been triaged by a lead label Nov 5, 2021
@jaredpar jaredpar added this to the 17.1 milestone Nov 5, 2021
@jaredpar jaredpar added the api-needs-work API needs work before it is approved, it is NOT ready for implementation label Nov 5, 2021
@jaredpar
Copy link
Member

jaredpar commented Nov 5, 2021

@333fred PTAL

@bernd5 could you please consider not pinging the issue multiple times in the same 24 hour period? Generally engineers look a couple of times a day and I generally triage all incoming issue once a day (although I do occasionally miss a day here and there). Multiple pings in the same day are unnecessary. If we have not responded after a couple of days then yes please do ping because we may have missed it.

@333fred
Copy link
Member

333fred commented Nov 5, 2021

There's no actual API change here, so I don't think there's really anything to look at from the public API side. I also don't see any problem with allowing the DataFlowAnalysis APIs to expose this information, so I think we should just review the linked PR.

@333fred 333fred removed the api-needs-work API needs work before it is approved, it is NOT ready for implementation label Nov 5, 2021
@333fred
Copy link
Member

333fred commented Nov 5, 2021

@AlekseyTs pointed out that we do have a bit of public API for C#-specific syntax nodes here: https://sourceroslyn.io/#Microsoft.CodeAnalysis.CSharp/Compilation/SyntaxTreeSemanticModel.cs,2250. So we'll want to add a new overload that takes a constructor initializer syntax node there.

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

bernd5 commented Nov 5, 2021

In my PR I added a specific overload in CSharpSemanticModel and in SyntaxTreeSemanticModel similar how it is implemented for statements and expressions (and it works quite great).
Both types are internal so it is not public API.

@333fred
Copy link
Member

333fred commented Nov 8, 2021

@bernd5 we still need to update the proposal to include extension overloads for https://sourceroslyn.io/#Microsoft.CodeAnalysis.CSharp/CSharpExtensions.cs,1038. We also probably want to do the same thing for the AnalyzeControlFlow API, to be consistent across the board.

@bernd5
Copy link
Contributor Author

bernd5 commented Nov 9, 2021

@333fred is this API-issue now ready for review?

@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 Nov 9, 2021
@AlekseyTs
Copy link
Contributor

I think we would want to add support for initializers of primary constructors as well.

@AlekseyTs
Copy link
Contributor

I know "primary constructors" only from records - but how can we define initializers for them?

Arguments for the base constructor invocation can be supplied on the base class. For example:

record Base(int x);
record Derived(int x, int y) : Base(x);

@bernd5
Copy link
Contributor Author

bernd5 commented Nov 9, 2021

Oh, yeah I forgot that - I'll add it, too.

@bernd5
Copy link
Contributor Author

bernd5 commented Nov 11, 2021

@AlekseyTs @333fred have you seen, that I updated the API proposal / PR?
Do you know when the next API review meeting is scheduled?

@333fred
Copy link
Member

333fred commented Nov 11, 2021

API review meets every two weeks. The next one is scheduled for next Thursday.

@333fred
Copy link
Member

333fred commented Nov 18, 2021

API Review

  • We need to make sure that we add tests for Extract Method to make sure it doesn't have negative behavior introduced with this method.
  • API shape is approved with the optional C#-specific overloads. The primary constructor overload needs to have its comments adjust to not imply that it's giving info about the base constructor itself, only the base call syntax.

@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 Nov 18, 2021
@bernd5
Copy link
Contributor Author

bernd5 commented Nov 20, 2021

I would change the comment to: "Analyze data-flow within a primary-constructor-base-type initializer.", okay?

@bernd5
Copy link
Contributor Author

bernd5 commented Nov 20, 2021

@333fred can you help me with the Extract-Method test?

@333fred
Copy link
Member

333fred commented Nov 20, 2021

@333fred can you help me with the Extract-Method test?

Not really, but @CyrusNajmabadi should be able to :)

@bernd5
Copy link
Contributor Author

bernd5 commented Nov 26, 2021

@CyrusNajmabadi can you help here?

@CyrusNajmabadi
Copy link
Member

Hi @bernd5 can you just go to src\EditorFeatures\CSharpTest\CodeActions\ExtractMethod\ExtractMethodTests.cs and add some tests that demonstrate the behavior of Extract-Method inside a constructor initializer. We should either see no change in behavior with your compiler change (so run the tests before/after to validate that), or we should see better behavior after your change. If things change and get worse, we'd need to discuss what was happening and what we should do about it. THanks!

@bernd5
Copy link
Contributor Author

bernd5 commented Nov 30, 2021

I can try - but I actually do not know how the tests work - or what they actually test.

What means for example:

[|b != true|]

or

|Rename:NewMethod|

I assume, [| describes the region start of the analysis - b != true is the analyzed expression - and |] terminates the analysis context.

AlekseyTs pushed a commit that referenced this issue Dec 1, 2021
…ntax` and `PrimaryConstructorBaseTypeSyntax` (#57576)

Fixes #57577.
JoeRobich added a commit that referenced this issue Dec 6, 2021
* Remove IDE0051 suppression

* Add LayoutKind

* LayoutKind.Sequential

Co-authored-by: Sam Harwell <sam@tunnelvisionlabs.com>

* Create azure-pipelines-integration-dartlab.yml

* Update azure-pipelines-integration-dartlab.yml

* [Async lightbulb] Move suppression fixes to 'Low' priority bucket

Addresses second item in #56843

Prior to this change, all the analyzers that are not high-pri are added to the normal priority bucket and executed at once. This PR further breaks down the normal priority bucket into two buckets as follow:

1. `Normal`: Analyzers that produce at least one fixable diagnostic, i.e. have a corresponding code fixer.
2. `Low`: Analyzers without any fixable diagnostics, i.e. have no corresponding code fixer. These diagnostics will only have Suppression/Configuration quick actions, which are lower priority and always show up at the bottom of the light bulb.

This new bucketing allows us to reduce the number of analyzers that are executed in the Normal priority bucket and hence generate all the non-suppression/configuration code fixes faster in async light bulb.

* Cache last project and compilationWithAnalyzers

* Update test as normal priority bucket has one lesser code action

* Make shallow checkout optional for integration test CI

* Move pipeline checkout skips up a layer.

* Look up VS bootstrapper from build

* Revert skipPipelinesCheckout change

* Comment out checkout: none for now

* Added matrix

* Target topic branch in DartLab.Templates

* Update number of machines to 4

* Run test agent elevated

* Update pipeline description

* Default VS branch to main

* Address feedback

* Exclude unnecessary sqlite assemblies

* Convert namespace to file scoped when typing semicolon

* Working on tests

* Add tests

* Add comments

* Add comments

* Revert

* Revert

* Make static

* Add tests

* Pass AnalysisKind instead of int

* Honor option, and also improve formatting with comment

* Fix comment

* Add tests

* Fix await completion for expression body lambda

* Add new parser/lexer to the StackTraceAnalyzer (#57598)

No functional changes, just moving to the new API and cleaning up unused code

* Add comments

* Make it possible to analyze the dataflow of `ConstructorInitializerSyntax` and `PrimaryConstructorBaseTypeSyntax` (#57576)

Fixes #57577.

* Snap 17.1 P2 (#58041)

* Add new parser/lexer to the StackTraceAnalyzer (#57598) (#58050)

No functional changes, just moving to the new API and cleaning up unused code

* Read SourceLink info and call service to retrieve source from there (#57978)

* Merge pull request #58100 from dotnet/dev/jorobich/skip-test

Skip flaky CommandLine.ArgumentParsing test

See #58077 for more details

Co-authored-by: Youssef Victor <31348972+Youssef1313@users.noreply.github.com>
Co-authored-by: Sam Harwell <sam@tunnelvisionlabs.com>
Co-authored-by: Brad White <4261702+bradselw@users.noreply.github.com>
Co-authored-by: Manish Vasani <mavasani@microsoft.com>
Co-authored-by: Joey Robichaud <jorobich@microsoft.com>
Co-authored-by: Brad White <bradwhit@microsoft.com>
Co-authored-by: gel@microsoft.com <gel@microsoft.com>
Co-authored-by: Cyrus Najmabadi <cyrusn@microsoft.com>
Co-authored-by: Sam Harwell <Sam.Harwell@microsoft.com>
Co-authored-by: Andrew Hall <andrha@microsoft.com>
Co-authored-by: Bernd Baumanns <baumannsbernd@gmail.com>
Co-authored-by: Allison Chou <allichou@microsoft.com>
Co-authored-by: CyrusNajmabadi <cyrus.najmabadi@gmail.com>
Co-authored-by: David Wengier <david.wengier@microsoft.com>
Co-authored-by: Gen Lu <genlu@users.noreply.github.com>
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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants