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

#66116 Long switch expression is duplicated in InferTypesWorker_DoNot… #71412

Merged
merged 6 commits into from
Dec 29, 2023

Conversation

vibeeshan025
Copy link
Contributor

@vibeeshan025 vibeeshan025 commented Dec 25, 2023

Fixes #66116.

Both InferTypesWorker_DoNotCallDirectly Methods will call another common method called InferTypesWorkerFromParent.

@microsoft-github-policy-service agree

@vibeeshan025 vibeeshan025 requested a review from a team as a code owner December 25, 2023 18:27
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Dec 25, 2023
@ghost ghost added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Dec 25, 2023
@CyrusNajmabadi
Copy link
Member

Due to the holidays, we likely won't be able to get to this for a couple of days/weeks. Thanks for your patience!

@vibeeshan025
Copy link
Contributor Author

vibeeshan025 commented Dec 25, 2023 via email

@vibeeshan025
Copy link
Contributor Author

Fixed failing test cases

@@ -395,6 +287,89 @@ private IEnumerable<TypeInferenceInfo> InferTypeInConstructorInitializer(Constru
return InferTypeInArgument(index, methods, argument, parentInvocationExpressionToTypeInfer: null);
}

private IEnumerable<TypeInferenceInfo> InferTypesWorkerFromParent(SyntaxNode parent, SyntaxToken? token = null, SyntaxNode node = null, ExpressionSyntax expression = null)
Copy link
Member

Choose a reason for hiding this comment

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

Can you make all the parameters non-optional plz.

Copy link
Member

Choose a reason for hiding this comment

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

parent, node, expression, token for the order please. thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, will make the change.

@@ -1684,35 +1659,28 @@ private IEnumerable<TypeInferenceInfo> InferTypeInMemberDeclarator(AnonymousObje
return SpecializedCollections.EmptyEnumerable<TypeInferenceInfo>();
}

private IEnumerable<TypeInferenceInfo> InferTypeInNameColon(NameColonSyntax nameColon, SyntaxToken previousToken)
private IEnumerable<TypeInferenceInfo> InferTypeInExpressionColon(ExpressionColonSyntax expressionColon, SyntaxToken? previousToken = null)
Copy link
Member

Choose a reason for hiding this comment

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

please don't use optional parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, let me change them

@@ -1883,19 +1851,24 @@ private IEnumerable<TypeInferenceInfo> InferTypeInExpressionColon(ExpressionColo
return null;
}

private IEnumerable<TypeInferenceInfo> InferTypeInNameColon(NameColonSyntax nameColon)
private IEnumerable<TypeInferenceInfo> InferTypeInNameColon(NameColonSyntax nameColon, SyntaxToken? previousToken = null)
Copy link
Member

Choose a reason for hiding this comment

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

no optional parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, let me change them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  protected override IEnumerable<TypeInferenceInfo> InferTypesWorker_DoNotCallDirectly(
      SyntaxNode node)
  {
      var expression = node as ExpressionSyntax;
      if (expression != null)
      {
          expression = expression.WalkUpParentheses();
          node = expression;
      }

      var parent = node.Parent;

      return InferTypesWorkerFromParent(parent, node, expression, node.GetFirstToken());
  }

Am I allowed to use like in the last line node.GetFristToken() as I believe a Node cannot be directly translated to a token.

Same as in other method I have to get the Node from token which is possible.

Copy link
Member

Choose a reason for hiding this comment

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

Am I allowed to use like in the last line node.GetFristToken() as I believe a Node cannot be directly translated to a token.

No. The apis are distinctly different. The token version is when we're inferring a type for a particular caret location (so the token represents what is immediately before the caret.

The node version is for when we're inferring what the type should be for an invalid node that we're generating a new symbol for. A toke should not be provided for this case.

Note: my feedback above was simply that the parameter not be optional. It can certainly be nullable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the explanation

@CyrusNajmabadi
Copy link
Member

LGTM with the removal of optional parameters. We prefer to avoid those in internal apis as it is both too easy to forget to pass something important, and it makes it unclear on the callsite what the semantics are. THanks!

@vibeeshan025
Copy link
Contributor Author

@CyrusNajmabadi All done.

{
return parent switch
{
// Path from SyntaxNode, node is never null
Copy link
Member

Choose a reason for hiding this comment

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

if you're interested, you can remove #nullable disable in another PR, and then fix all this up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i will work on this as another PR.
Thanks for the input.

@CyrusNajmabadi
Copy link
Member

thanks!

@CyrusNajmabadi CyrusNajmabadi merged commit bd598e3 into dotnet:main Dec 29, 2023
28 checks passed
@ghost ghost added this to the Next milestone Dec 29, 2023
@CyrusNajmabadi
Copy link
Member

Thanks!

@Cosifne Cosifne added this to the 17.9 P3 milestone Jan 9, 2024
@CyrusNajmabadi
Copy link
Member

I'm getting these crashes now in the latest vs bits:

   at Microsoft.CodeAnalysis.CSharp.CSharpTypeInferenceService.TypeInferrer.InferTypesWorkerFromParent(SyntaxNode parent,SyntaxNode node,ExpressionSyntax expression,Nullable`1 token)
   at Microsoft.CodeAnalysis.CSharp.CSharpTypeInferenceService.TypeInferrer.InferTypesWorker_DoNotCallDirectly(SyntaxNode node)

we may have to roll this back, unless you want to take a look and fix asap @vibeeshan025

@vibeeshan025
Copy link
Contributor Author

Let me start working on this tomorrow

@CyrusNajmabadi
Copy link
Member

looks like a null ref. likely one of the token.Value calls. the code is pretty unsafe in its current form.

@vibeeshan025
Copy link
Contributor Author

Let me work on the tomorrow. I believe I can finish it tomorrow.

@CyrusNajmabadi
Copy link
Member

Thanks!

CyrusNajmabadi added a commit to CyrusNajmabadi/roslyn that referenced this pull request Jan 12, 2024
This reverts commit bd598e3, reversing
changes made to a5daa7d.
@CyrusNajmabadi
Copy link
Member

Crash has made it into VS. Backporting the reversion as well.

CyrusNajmabadi added a commit to CyrusNajmabadi/roslyn that referenced this pull request Jan 29, 2024
This reverts commit bd598e3, reversing
changes made to a5daa7d.
arunchndr added a commit that referenced this pull request Feb 1, 2024
Revert "Merge pull request #71412 from vibeeshan025/main
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Long switch expression is duplicated in InferTypesWorker_DoNotCallDirectly overloads
3 participants