TargetKind has no value for representing a type parameter #49796
Comments
|
I may implement this, as long as I have some guidance on this, as AFAIK we are going to have to make changes in many places and I am not sure if I know everything that has to been changed. I know of the following places where we are going to have to change:
Is there anywhere else? |
|
The Adding members to an enum makes existing It'll be a breaking change to make it so, but at least it should only be one break. |
lrhn
added
area-analyzer
|
Are you proposing that this change be done in the same PR or should we open a different PR for this? Also, should we open a tracking issue? |
|
CC @bwilkerson @pq for the idea of changing TargetKind from an enum. I think it's a good idea. |
|
I have no objection to changing it. |
I think that covers it! We accept PRs. |
|
Thanks, @srawlins. Do I turn the enum into a class in the same PR as the one including the new value? |
|
I think that would be fine; they're related insofar as: adding |
|
SGTM too! |
|
I am working on it! |
|
https://dart-review.googlesource.com/c/sdk/+/258200 I am not sure if I did everything right... testing it through the analyzer example worked for me, but I am having some issues setting up a reliable environment for dart-sdk, so I am afraid that I didn't test exactly as expected. |
|
@bwilkerson do you think it is a breaking change to make an enum into a const class with a constructor? (Of course given that we have no definition of breaking change and Hyrum's Law etc.) I can't think of a way that it is breaking. |
|
There's a chance that it isn't. I was assuming that it was because someone I respect said that it was, but it's possible that they were wrong. I can't think of any case where the enum could have been used in a way that isn't compatible with the class. |
If someone uses the enum in a switch the static analysis won't required a default case if it is exhaustive. In this specific case, won't add a new value break the switch? Although I think it's improbable that someone is using it this way. |
|
@mateusfccp you've got it. It will trigger "body_might_complete_normally" when flow analysis (or definite assignment) detects the missing int f2(E2 e2) {
switch (e2) {
case E2.one: return 1;
case E2.two: return 2;
}
} |
|
I think it might be most pragmatic to release this "breaking change" in a non-major release. Similar to the Dart and Flutter SDKs. In this code, if E is an enum with two values: int f(E e) {
switch (e) {
case E.one: return 1;
case E.two: return 2;
default: return 3;
}
}we can include the So we can pre-migrate clients to use |
|
Pre-migrating clients, where we can, sounds good to me. I'm not sure how widely That said, the last time we made a breaking change without bumping the major version number (we didn't realize it was a breaking change) it caused a lot of pain for the community. While this will hopefully be less impactful, I'm still not sure it's a wise thing to do. Of course, releasing a new major version won't be without pain, so we need to balance the two. |
|
Yeah I believe releasing a new major version is more pain. |
|
I commented this on the PR, but no one answered, so maybe here it will get more visibility. I am trying to fix the tests, but I am having problem running them in my local machine. Maybe I didn't set up properly the environment. I followed the "Contributing" and "Building" documentation and I can build the source with no issues. However, when running I automatically though that the issue was that I was using the local Using it, I got the following error: For some reason, the executable is trying to get the snapshots from Now, I am getting the following: I feel like I missed something when setting up the development environment... Is there any documentation that maybe I missed? |
I'm sorry I missed your question on the PR, sometimes I miss the notifications when PRs are updated. Thanks for reaching out again.
It looks to me like there's a version mismatch between the version of package Personally, I never use I'm not sure how to resolve the issue you're seeing, but perhaps running the test files directly will work around the problem. |
…rgetKind to represent type parameter Bug: #49796 Change-Id: Ide144ceb57bae94a71b9d1a7ec841d03363fc121 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/258200 Commit-Queue: Brian Wilkerson <brianwilkerson@google.com> Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
…ue in TargetKind to represent type parameter" This reverts commit 82143e6. Reason for revert: This commit appears to have negatively affected the analyze benchmark: * https://golem.corp.goog/Revision?repository=flutter-analyze&team=dartanalyzer&revision=114193 We need to revert and analyze it. Original change's description: > [analyzer][meta] Refactor TargetKind to be a class, add a value in TargetKind to represent type parameter > > Bug: #49796 > Change-Id: Ide144ceb57bae94a71b9d1a7ec841d03363fc121 > Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/258200 > Commit-Queue: Brian Wilkerson <brianwilkerson@google.com> > Reviewed-by: Brian Wilkerson <brianwilkerson@google.com> # Not skipping CQ checks because original CL landed > 1 day ago. Bug: #49796 Change-Id: I6ec04ffe8d85c417d138626ffa4f1a7ac7dafe2b Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/268380 Reviewed-by: Samuel Rawlins <srawlins@google.com> Commit-Queue: Samuel Rawlins <srawlins@google.com> Reviewed-by: Konstantin Shcheglov <scheglov@google.com> Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
The Dart specification says:
This is also the way it's implemented, such that
is valid code.
However,
TargetKindlacks any value that may represent this case.I propose creating a
TargetKind.typeParameterto fulfill this necessity.The text was updated successfully, but these errors were encountered: