Completion missing members of lambda parameter in fault tolerance case#34312
Completion missing members of lambda parameter in fault tolerance case#34312ivanbasov merged 17 commits intodotnet:masterfrom
Conversation
src/Workspaces/Core/Portable/Recommendations/AbstractRecommendationService.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/Core/Portable/Recommendations/AbstractRecommendationService.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
whooboy. this is complex.
- Consider doc'ing this.
- should we pull these type parameters up to the class? Does either language ever call this same method with a different instantiation? #Resolved
There was a problem hiding this comment.
this whole method needs comments helping explain what is going on. i.e.: // this code is to help give intellisense in the following case: .... And then explain what we're looking for and what this is working around. #Resolved
There was a problem hiding this comment.
i'm going to hold off until then, aas that will really help me understand what's going on here and why it's appropriate. Ping me whne you think it's good to look again! #Resolved
There was a problem hiding this comment.
Single is very dangerous. We should verify we only have one ref before doing this. you never know if a future compiler change might break this. #Resolved
|
@CyrusNajmabadi , @dpoeschl , could you please review? #Resolved |
| <PackageReference Include="Microsoft.VisualStudio.Shell.15.0" Version="$(MicrosoftVisualStudioShell150Version)" /> | ||
| <PackageReference Include="Microsoft.VisualStudio.ComponentModelHost" Version="$(MicrosoftVisualStudioComponentModelHostVersion)" /> | ||
| <PackageReference Include="Newtonsoft.Json" Version="$(NewtonsoftJsonVersion)" /> | ||
| <PackageReference Include="StreamJsonRpc" Version="$(StreamJsonRpcVersion)" /> |
There was a problem hiding this comment.
why do we need this? #Resolved
There was a problem hiding this comment.
| protected abstract bool TryGetOrdinalInArgumentList(SyntaxNode argumentOpt, out int ordinalInInvocation); | ||
|
|
||
| // This code is to help give intellisense in the following case: | ||
| // query.Include(a => a.SomeProerty).ThenInclude(a => a. |
There was a problem hiding this comment.
Proerty #Resolved
| // Cannot proceed without DeclaringSyntaxReferences. | ||
| // We expect that there is a single DeclaringSyntaxReferences in the scenario. | ||
| // If anything changes on the compiler side, the approach should be revised. | ||
| if (containingMethod.DeclaringSyntaxReferences.IsDefaultOrEmpty || containingMethod.DeclaringSyntaxReferences.Length > 1) |
There was a problem hiding this comment.
DeclaringSyntaxReferences is never Default. So you could just simple this to .DeclaringSyntaxReferences.Length != 1 #Resolved
| continue; | ||
| } | ||
|
|
||
| type = type.GetAllTypeArguments()[ordinalInLambda]; |
There was a problem hiding this comment.
allTypeArguments[ordinalInLambda] #Resolved
|
|
||
| Return New VisualBasicRecommendationServiceRunner(context, filterOutOfScopeLocals, cancellationToken) | ||
| End Function | ||
|
|
There was a problem hiding this comment.
remove. #Resolved
| End Sub | ||
|
|
||
| Public Overrides Function GetSymbols() As ImmutableArray(Of ISymbol) | ||
|
|
There was a problem hiding this comment.
remove... #Resolved
| End Function | ||
|
|
||
| Protected Overrides Function IsInvocationExpression(syntaxNode As SyntaxNode) As Boolean | ||
|
|
| End Function | ||
|
|
||
| Protected Overrides Function IsLambdaExpression(syntaxNode As SyntaxNode) As Boolean | ||
|
|
There was a problem hiding this comment.
I'm not a fan of those empty lines. I just found them in the original code. Was it an obsolete code style? #Resolved
There was a problem hiding this comment.
yeah... that's super bizarre... i'm not sure how that happened. def can remove :) (and can get rid of all my comments). #Resolved
There was a problem hiding this comment.
OK. Thanks! Will remove all the empty lines #Resolved
| End Function | ||
|
|
||
| Protected Overrides Function TryGetOrdinalInArgumentList(argumentOpt As SyntaxNode, ByRef ordinalInInvocation As Integer) As Boolean | ||
|
|
|
|
||
| ordinalInInvocation = -1 | ||
| Return False | ||
| End Function |
There was a problem hiding this comment.
so, fwiw, i think you could use ISyntaxFacts and not need the top two methods. You could also move this method into the base type. #Resolved
| End Function | ||
|
|
||
| Private Function GetUnqualifiedSymbolsForLabelContext() As ImmutableArray(Of ISymbol) | ||
|
|
There was a problem hiding this comment.
remove. this style is not used at all in the IDE. #Resolved
| End Function | ||
|
|
||
| Private Function IsOrContainsValidAccessibleClass(namespaceOrTypeSymbol As INamespaceOrTypeSymbol, within As ISymbol) As Boolean | ||
|
|
There was a problem hiding this comment.
please undo all of this :) it's just not the IDE style (i also don't understand why we'd want it). #Resolved
| return default(IIncludableQueryable<TEntity, TProperty>); | ||
| } | ||
| } | ||
| }]]> |
There was a problem hiding this comment.
could you extract this into a constant and just include that in all these tests. it really bloats to have to have it in each test. #Resolved
There was a problem hiding this comment.
it also makes it harder to tell how each test differs. #Resolved
|
|
||
| Friend Class Registration | ||
| Public Property Activities As ICollection(Of Activity) | ||
| End Class |
There was a problem hiding this comment.
same point here. #Resolved
| { | ||
| var builder = ArrayBuilder<ITypeSymbol>.GetInstance(); | ||
| var expressionSymbol = _context.SemanticModel.Compilation.GetTypeByMetadataName(typeof(Expression<>).FullName); | ||
| var funcSymbol = _context.SemanticModel.Compilation.GetTypeByMetadataName(typeof(Func<>).FullName); |
There was a problem hiding this comment.
we need to be resilient to these not being around.
it's rare, but it can definitely happen during solution load (when we don't have all references properly attached). Either we can bail here if they're null, or we can check them in the loop accordingly. #Resolved
| cancellationToken As CancellationToken) As AbstractRecommendationServiceRunner(Of VisualBasicSyntaxContext) | ||
| Return New VisualBasicRecommendationServiceRunner(context, filterOutOfScopeLocals, cancellationToken) | ||
| End Function | ||
| End Class |
There was a problem hiding this comment.
can just revert this file. #Resolved
CyrusNajmabadi
left a comment
There was a problem hiding this comment.
LGTM. would like test cleanup, and we need some null checks. I don't need to review again. Tnx!
|
@dpoeschl please review |
|
@genlu could you please review? |
src/Workspaces/Core/Portable/Recommendations/AbstractRecommendationServiceRunner.cs
Outdated
Show resolved
Hide resolved
|
Nice work @ivanbasov !! |
Fix #8237