[M] Change in dart/sdk[main]: [analyzer_plugin] Fixes writing recursive type parameter
Felipe Morschel (Gerrit)
Felipe Morschel has uploaded the change for review![]( "Open in Gerrit")
Commit message
[analyzer_plugin] Fixes writing recursive type parameter
Change information
- M pkg/analyzer_plugin/lib/src/utilities/change_builder/change_builder_dart.dart
- M pkg/analyzer_plugin/test/src/utilities/change_builder/change_builder_dart_test.dart
Related details
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 |
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. |
Samuel Rawlins (Gerrit)
Samuel Rawlins voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
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. |
Brian Wilkerson (Gerrit)
Brian Wilkerson added 4 comments![]( "Open in Gerrit")
var typeParameters = {It would be good to document the reason for this code because it isn't obvious, IMO.
I think the idea is that if we ever run into a type argument that's a type parameter, we add it to the list of known type parameters so that if we encounter it again we'll stop recursing.
Let me know if I'm wrong.
If so, it feels imprecise and confusing to pretend that a type parameter in an argument list is in scope when it isn't actually in scope. This feels like brittle code, but I can't construct a counter example, so I can't prove that it won't work in all cases.
(The more common solution, at least in our code bases, would be to keep a second list containing all of the types we've looked at to get to the current `type`, and use that to detect when we've recursed. I don't know whether that's necessary here, but it seems easier to reason about that approach.)
if (argument.element == element) {Similarly this code could use a comment.
In this case I'm less convinced that it's a valid approach. It catches the case where the bound of a type parameter is the same as the type we're trying to write, but if the circularity is indirect rather than direct, then I think this code will miss it. For example
```dart
class A<S extends B<S>> {}
class B<T extends A<T>> {}
```
(although I suspect that that won't actually exhibit the bug I think I'm seeing, it's just to try to make my comments more concrete).
I'm not sure how to prevent recursion in this case, but the first thing that comes to mind is to keep a stack of the type parameters whose bound we have had to use so that if we ever need to get the bound of a type parameter we can check to see whether we've already done so and at that point we can abort. (Though I suspect that the right place to recover from the recursion is at the point where we're writing out the bound the first time, so the method that finds circularities probably needs to be outside the methods that actually write the type.)
Please let me know if this actually is guaranteed to catch all forms of recursion.
int offset = 0,I took me a minute to understand why we need this.
But given that we do, it seems like either (a) this should be required so that all clients are forced to pass the right offset, or (b) we should remove this API and force clients to specify the offset via `addInsertion` or `addReplacement`.
I think I'd prefer the latter, even though I know that removing the API is a breaking change (which means deprecating it first and then removing it later), but I haven't looked into the implications of doing so.
Related details
- 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 4 comments![]( "Open in Gerrit")
Votes added by Felipe Morschel
| Auto-Submit | +1 |
4 comments
@soloTestYou'll need to remove this.
Thanks for cathing it!
var typeParameters = {It would be good to document the reason for this code because it isn't obvious, IMO.
I think the idea is that if we ever run into a type argument that's a type parameter, we add it to the list of known type parameters so that if we encounter it again we'll stop recursing.
Let me know if I'm wrong.
If so, it feels imprecise and confusing to pretend that a type parameter in an argument list is in scope when it isn't actually in scope. This feels like brittle code, but I can't construct a counter example, so I can't prove that it won't work in all cases.
(The more common solution, at least in our code bases, would be to keep a second list containing all of the types we've looked at to get to the current `type`, and use that to detect when we've recursed. I don't know whether that's necessary here, but it seems easier to reason about that approach.)
I think the idea is that if we ever run into a type argument that's a type parameter, we add it to the list of known type parameters so that if we encounter it again we'll stop recursing.
Yes, we already did that for `FunctionType`s but not for `InterfaceType`s so I'm doing the same here.
The more common solution...
I saw that somewhere and I thought about doing this but it felt a little too inconsistent given we did the same above. I'm open to do that if you prefer though.
I can't construct a counter example, so I can't prove that it won't work in all cases.
Since I asked for your help at the other comment, could you please take a look at this too @eer...@google.com? Thanks!
if (argument.element == element) {Similarly this code could use a comment.
In this case I'm less convinced that it's a valid approach. It catches the case where the bound of a type parameter is the same as the type we're trying to write, but if the circularity is indirect rather than direct, then I think this code will miss it. For example
```dart
class A<S extends B<S>> {}
class B<T extends A<T>> {}
```
(although I suspect that that won't actually exhibit the bug I think I'm seeing, it's just to try to make my comments more concrete).I'm not sure how to prevent recursion in this case, but the first thing that comes to mind is to keep a stack of the type parameters whose bound we have had to use so that if we ever need to get the bound of a type parameter we can check to see whether we've already done so and at that point we can abort. (Though I suspect that the right place to recover from the recursion is at the point where we're writing out the bound the first time, so the method that finds circularities probably needs to be outside the methods that actually write the type.)
Please let me know if this actually is guaranteed to catch all forms of recursion.
I tried thinking of ways to represent that too but for your example, we get `type_argument_not_matching_bounds` because we need to represent that `S extends A` too and `T extends B` too but we currently can't do that.
So if we could come up with a test case for this I would definitely feel more comfortable writing a more complex approach here.
Maybe @eer...@google.com can think of something?
int offset = 0,I took me a minute to understand why we need this.
But given that we do, it seems like either (a) this should be required so that all clients are forced to pass the right offset, or (b) we should remove this API and force clients to specify the offset via `addInsertion` or `addReplacement`.
I think I'd prefer the latter, even though I know that removing the API is a breaking change (which means deprecating it first and then removing it later), but I haven't looked into the implications of doing so.
Yeah, I wasn't sure if you'd prefer to keep this or to deprecate it, so I vouched for the less breaking version.
I'm fine with deprecating it.
Related details
- Brian Wilkerson
- Erik Ernst
- 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 added 2 comments![]( "Open in Gerrit")
if (argument.element == element) {Felipe MorschelSimilarly this code could use a comment.
In this case I'm less convinced that it's a valid approach. It catches the case where the bound of a type parameter is the same as the type we're trying to write, but if the circularity is indirect rather than direct, then I think this code will miss it. For example
```dart
class A<S extends B<S>> {}
class B<T extends A<T>> {}
```
(although I suspect that that won't actually exhibit the bug I think I'm seeing, it's just to try to make my comments more concrete).I'm not sure how to prevent recursion in this case, but the first thing that comes to mind is to keep a stack of the type parameters whose bound we have had to use so that if we ever need to get the bound of a type parameter we can check to see whether we've already done so and at that point we can abort. (Though I suspect that the right place to recover from the recursion is at the point where we're writing out the bound the first time, so the method that finds circularities probably needs to be outside the methods that actually write the type.)
Please let me know if this actually is guaranteed to catch all forms of recursion.
I tried thinking of ways to represent that too but for your example, we get `type_argument_not_matching_bounds` because we need to represent that `S extends A` too and `T extends B` too but we currently can't do that.
So if we could come up with a test case for this I would definitely feel more comfortable writing a more complex approach here.
Maybe @eer...@google.com can think of something?
I'm aware that it's invalid code, but our tools have to work with invalid code all the time (otherwise they can get stuck in infinite loops 😊), so the fact that there are errors in the code doesn't necessarily mean that it's a bad test case.
int offset = 0,Felipe MorschelI took me a minute to understand why we need this.
But given that we do, it seems like either (a) this should be required so that all clients are forced to pass the right offset, or (b) we should remove this API and force clients to specify the offset via `addInsertion` or `addReplacement`.
I think I'd prefer the latter, even though I know that removing the API is a breaking change (which means deprecating it first and then removing it later), but I haven't looked into the implications of doing so.
Yeah, I wasn't sure if you'd prefer to keep this or to deprecate it, so I vouched for the less breaking version.
I'm fine with deprecating it.
I looked into it a little more. We're using it in 7 places, and in all of those we're using it to decide whether to add an edit (insertion or replacement).
IIRC, calling `addInsertion` or `addReplacement` and then not providing any text will still create an edit, and in the case of `addReplacement` it will effectively be a deletion.
Given that, I'm now convinced that we really do need this method. I'd still prefer that we require an offset because I believe that doing so will help prevent bugs.
Related details
- Erik Ernst
- Felipe Morschel
- 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")
| Code-Review | +1 |
Related details
- Erik Ernst
- Felipe Morschel
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")
| Code-Review | +1 |
Related details
- Brian Wilkerson
- Erik Ernst
- 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 1 comment![]( "Open in Gerrit")
Votes added by Felipe Morschel
| Auto-Submit | +1 |
1 comment
Thanks for the bots, @sraw...@google.com! I've fixes the problem and ran all DAS and analyzer_plugin tests locally. Everything seems to be working fine now.
Related details
- Brian Wilkerson
- Erik Ernst
- 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")
- Erik Ernst
- Felipe Morschel
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |