[M] Change in dart/sdk[main]: [DAS] Fixes regression in completion for unnamed parameters in closures
Felipe Morschel (Gerrit)
Felipe Morschel has uploaded the change for review![]( "Open in Gerrit")
Commit message
[DAS] Fixes regression in completion for unnamed parameters in closures
Change information
- M pkg/analysis_server/lib/src/services/completion/dart/in_scope_completion_pass.dart
- M pkg/analysis_server/lib/src/services/completion/dart/utilities.dart
- M pkg/analysis_server/test/client/completion_driver_test.dart
- M pkg/analysis_server/test/lsp/completion_dart_test.dart
- M pkg/analysis_server/test/services/completion/dart/completion_test.dart
- M pkg/analysis_server/test/services/completion/dart/declaration/closure_test.dart
- M pkg/analysis_server/test/services/completion/dart/location/argument_list_test.dart
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danny Tuppeny (Gerrit)
Danny Tuppeny added 2 comments![]( "Open in Gerrit")
bool inlcudeDisplayText = false;nit: typo `inlcude`
Maybe also include a comment explaining what this does like the others.
Is this actually used (set) anywhere?
f((p1) => ^,);I think originally these would start at 0 instead of 1. I don't know if it matters, but if there is other code generating similar names that still start from 0, maybe this should be consistent?
Related details
- Brian Wilkerson
- Felipe Morschel
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel voted and added 2 comments![]( "Open in Gerrit")
Votes added by Felipe Morschel
| Auto-Submit | +1 |
2 comments
nit: typo `inlcude`
Maybe also include a comment explaining what this does like the others.
Is this actually used (set) anywhere?
I'll remove this, I was testing this out but I found other tests that filled this role for this fix. Thanks! If anyone else wants to re add this in the future is simple enough to do so.
I think originally these would start at 0 instead of 1. I don't know if it matters, but if there is other code generating similar names that still start from 0, maybe this should be consistent?
- Brian Wilkerson
- Danny Tuppeny
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins voted and added 1 comment![]( "Open in Gerrit")
Votes added by Samuel Rawlins
| Code-Review | +1 |
| Commit-Queue | +2 |
1 comment
Related details
- Brian Wilkerson
- Danny Tuppeny
- Felipe Morschel
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel voted and added 2 comments![]( "Open in Gerrit")
Votes added by Felipe Morschel
| Auto-Submit | +1 |
2 comments
(item1.sortText == item2.sortText)This ensures that when two completions have the same priority (as the ones presented by the new LSP test) then we sort them by the text we show to the user.
expectCompletions: ['^2.1.0', '^2.3.4'],Necessary after the change at `pkg/analysis_server/test/lsp/completion.dart`
Related details
- Brian Wilkerson
- Danny Tuppeny
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins voted Code-Review+1![]( "Open in Gerrit")
- Brian Wilkerson
- Danny Tuppeny
- Felipe Morschel
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danny Tuppeny (Gerrit)
Danny Tuppeny added 1 comment![]( "Open in Gerrit")
(item1.sortText == item2.sortText)This ensures that when two completions have the same priority (as the ones presented by the new LSP test) then we sort them by the text we show to the user.
Do we want this? If I understand correctly, this isn't sorting what the user sees, it's just sorting what the tests look at. I think it would be best if tests could assert the ordering of the returned items even when the sort text is the same - I'm not sure what the advantage of doing this here is.
Related details
- Brian Wilkerson
- Felipe Morschel
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel added 1 comment![]( "Open in Gerrit")
(item1.sortText == item2.sortText)Danny TuppenyThis ensures that when two completions have the same priority (as the ones presented by the new LSP test) then we sort them by the text we show to the user.
Do we want this? If I understand correctly, this isn't sorting what the user sees, it's just sorting what the tests look at. I think it would be best if tests could assert the ordering of the returned items even when the sort text is the same - I'm not sure what the advantage of doing this here is.
I'm not sure we could handle how the clients sort the text after `sortText` sorting (which we expect them to but obviously could be ignored).
The latest [failing bot](https://logs.chromium.org/logs/dart/buildbucket/cr-buildbucket/8703375698794674353/+/u/test_results/new_test_failures__logs_) showed an error at `pkg/analysis_server/test/lsp/completion_dart_test.dart:1341` with the sorting being different than what I wrote. But locally, if I swapped them I got the error and this way I didn't (not sure why).
But the thing is, I don't expect this difference to be relevant to the users considering the clients use at least `sortText`. If we wan't to be sure, then we probably need to enhance that value for the cases I had to change here (that and related tests and at `pkg/analysis_server/test/lsp/completion_yaml_test.dart`). I don't see any real value between sorting them "better" to the end user at this point, but of course I could be wrong.
Related details
- Brian Wilkerson
- Danny Tuppeny
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danny Tuppeny (Gerrit)
Danny Tuppeny added 1 comment![]( "Open in Gerrit")
(item1.sortText == item2.sortText)Danny TuppenyThis ensures that when two completions have the same priority (as the ones presented by the new LSP test) then we sort them by the text we show to the user.
Felipe MorschelDo we want this? If I understand correctly, this isn't sorting what the user sees, it's just sorting what the tests look at. I think it would be best if tests could assert the ordering of the returned items even when the sort text is the same - I'm not sure what the advantage of doing this here is.
I'm not sure we could handle how the clients sort the text after `sortText` sorting (which we expect them to but obviously could be ignored).
The latest [failing bot](https://logs.chromium.org/logs/dart/buildbucket/cr-buildbucket/8703375698794674353/+/u/test_results/new_test_failures__logs_) showed an error at `pkg/analysis_server/test/lsp/completion_dart_test.dart:1341` with the sorting being different than what I wrote. But locally, if I swapped them I got the error and this way I didn't (not sure why).
But the thing is, I don't expect this difference to be relevant to the users considering the clients use at least `sortText`. If we wan't to be sure, then we probably need to enhance that value for the cases I had to change here (that and related tests and at `pkg/analysis_server/test/lsp/completion_yaml_test.dart`). I don't see any real value between sorting them "better" to the end user at this point, but of course I could be wrong.
Odd, I would expect the sorting to be deterministic. I'd be interested to understand why it was different for you, but I don't think it's worth holding this fix up.
Related details
- Brian Wilkerson
- Felipe Morschel
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danny Tuppeny (Gerrit)
Danny Tuppeny added 1 comment![]( "Open in Gerrit")
(item1.sortText == item2.sortText)Danny TuppenyThis ensures that when two completions have the same priority (as the ones presented by the new LSP test) then we sort them by the text we show to the user.
Felipe MorschelDo we want this? If I understand correctly, this isn't sorting what the user sees, it's just sorting what the tests look at. I think it would be best if tests could assert the ordering of the returned items even when the sort text is the same - I'm not sure what the advantage of doing this here is.
Danny TuppenyI'm not sure we could handle how the clients sort the text after `sortText` sorting (which we expect them to but obviously could be ignored).
The latest [failing bot](https://logs.chromium.org/logs/dart/buildbucket/cr-buildbucket/8703375698794674353/+/u/test_results/new_test_failures__logs_) showed an error at `pkg/analysis_server/test/lsp/completion_dart_test.dart:1341` with the sorting being different than what I wrote. But locally, if I swapped them I got the error and this way I didn't (not sure why).
But the thing is, I don't expect this difference to be relevant to the users considering the clients use at least `sortText`. If we wan't to be sure, then we probably need to enhance that value for the cases I had to change here (that and related tests and at `pkg/analysis_server/test/lsp/completion_yaml_test.dart`). I don't see any real value between sorting them "better" to the end user at this point, but of course I could be wrong.
Odd, I would expect the sorting to be deterministic. I'd be interested to understand why it was different for you, but I don't think it's worth holding this fix up.
I had a quick look but couldn't explain why the sort would differ - for me it seemed to always come out as the server set it if the `sortText`s are the same (including if I flipped `_addClosureSuggestion` to put the `=>` version first).
However, the sort we use in the handler doesn't appear to be stable, so it's not guaranteed to be the same and we probably shouldn't have the tests depend on it, so I think this change probably makes sense (either that, or making it stable in the handler, but since the client is going to re-sort and may be unstable, that's probably not worth it).
Brian Wilkerson (Gerrit)
Brian Wilkerson added 1 comment![]( "Open in Gerrit")
switchI can't think of any reasonable use for `true`, `false`, or `const`, but `null` should be included if the parameter is nullable (and I think it won't be, so please add the following as a test, but without the comment):
```dart
typedef void callback(int k);
void x(callback? q){}
void r() {
callback v;
x(^); // null is valid here
}
```
and I can see a use case for a `switch` expression, so that should be offered
```dart
typedef void callback(int k);
void x(callback q){}
void r(int i) {
callback u, v;
x(^); // could be
// switch (i) {
// < 0 => u,
// _ => v,
// }
}
```
Related details
- Felipe Morschel
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel voted and added 3 comments![]( "Open in Gerrit")
Votes added by Felipe Morschel
| Auto-Submit | +1 |
3 comments
// TODO(FMorschel): Determine if the expected type is bool and onlyI'll do this on a follow up CL since this might be a bigger change to the tests expected outputs.
// TODO(FMorschel): Determine if the parameter type has a constantI'm not sure I can do that reliably yet but I'll look into it in another CL too.
I can't think of any reasonable use for `true`, `false`, or `const`, but `null` should be included if the parameter is nullable (and I think it won't be, so please add the following as a test, but without the comment):
```dart
typedef void callback(int k);
void x(callback? q){}
void r() {
callback v;
x(^); // null is valid here
}
```
and I can see a use case for a `switch` expression, so that should be offered
```dart
typedef void callback(int k);
void x(callback q){}
void r(int i) {
callback u, v;
x(^); // could be
// switch (i) {
// < 0 => u,
// _ => v,
// }
}
```
- Brian Wilkerson
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins voted and added 1 comment![]( "Open in Gerrit")
Votes added by Samuel Rawlins
| Code-Review | +1 |
- Brian Wilkerson
- Felipe Morschel
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Wilkerson (Gerrit)
Brian Wilkerson added 3 comments![]( "Open in Gerrit")
Sorry, I commented on this a while back, but appear to have forgotten to send my comments.
null`null` is a reasonable suggestion. The parameter has type `dynamic`, which is nullable even though it doesn't have a `?` after it.
nullDitto: the parameter is nullable because it's `dynamic`.
Related details
- Felipe Morschel
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel voted and added 2 comments![]( "Open in Gerrit")
Votes added by Felipe Morschel
| Auto-Submit | +1 |
2 comments
`null` is a reasonable suggestion. The parameter has type `dynamic`, which is nullable even though it doesn't have a `?` after it.
Done
Ditto: the parameter is nullable because it's `dynamic`.
- Brian Wilkerson
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Wilkerson (Gerrit)
Brian Wilkerson voted![]( "Open in Gerrit")
- Felipe Morschel
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel voted and added 1 comment![]( "Open in Gerrit")
Votes added by Felipe Morschel
| Auto-Submit | +1 |
1 comment
Felipe MorschelI'm opening a comment here after also pinging you on the issue so you can find this either here or on GitHub.
@brianwi...@google.com @sraw...@google.com
I had to rebase and refactor this a bit because of https://dart-review.googlesource.com/c/sdk/+/453041. We should be good now.
Related details
- Brian Wilkerson
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins voted Code-Review+1![]( "Open in Gerrit")
- Brian Wilkerson
- Felipe Morschel
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Wilkerson (Gerrit)
Brian Wilkerson voted![]( "Open in Gerrit")
- Felipe Morschel
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Commit Queue (Gerrit)
Commit Queue submitted the change![]( "Open in Gerrit")
Change information
[DAS] Fixes regression in completion for unnamed parameters in closures
- M pkg/analysis_server/lib/src/services/completion/dart/in_scope_completion_pass.dart
- M pkg/analysis_server/lib/src/services/completion/dart/keyword_helper.dart
- M pkg/analysis_server/lib/src/services/completion/dart/utilities.dart
- M pkg/analysis_server/test/client/completion_driver_test.dart
- M pkg/analysis_server/test/lsp/completion.dart
- M pkg/analysis_server/test/lsp/completion_dart_test.dart
- M pkg/analysis_server/test/lsp/completion_yaml_test.dart
- M pkg/analysis_server/test/services/completion/dart/completion_test.dart
- M pkg/analysis_server/test/services/completion/dart/declaration/closure_test.dart
- M pkg/analysis_server/test/services/completion/dart/location/argument_list_test.dart
- M pkg/analysis_server/test/services/completion/dart/location/function_invocation_test.dart
Code-Review: +1 by Samuel Rawlins, +1 by Brian Wilkerson
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel added 1 comment![]( "Open in Gerrit")
Felipe MorschelI'm opening a comment here after also pinging you on the issue so you can find this either here or on GitHub.
@brianwi...@google.com @sraw...@google.com
I had to rebase and refactor this a bit because of https://dart-review.googlesource.com/c/sdk/+/453041. We should be good now.
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |