Repository navigation
[Dart Fix] Proposal: expose the static type of a fragment #52664
Description
I'm writing a dart fix that migrates a constructor argument from a double? to a TextScaler?, the migrated code is hard to read in some cases.
Original Code:
Text(textScaleFactor: scaleFactor) where scaleFactor is an expression that can either be a double?, double, or null.
Desired migrated code:
if scaleFactor's static type is double? => Text(textScaler: switch (scaleFactor) { null => null, final factor => TextScaler.linear(factor), })
if scaleFactor's static type is double => Text(textScaler: TextScaler.linear(scaleFactor)
if scaleFactor's static type is null => Text()
Current migrated code:
Text(textScaler: switch (scaleFactor) { null => null, final factor => TextScaler.linear(factor), })When scaleFactor is a double the current code is a lot harder to read than necessary, e.g.:
Text(textScaler: switch (2.0) { null => null, final factor => TextScaler.linear(factor), })
Activity
bwilkerson commentedon Jun 10, 2023
Supporting arbitrary type checks would require performing resolution of code snippets (type annotations) taken from the
fix_data.yamlfile. I'm not sure how feasible that is.However, I doubt very much that you're likely to encounter an expression of type
Nullexcept when the expression is thenullliteral, and I believe we can check that case easily enough today.That just leaves the question of whether the type is nullable, which seems like an easier thing to support.
The question then is whether transform authors need full control when the value is nullable or whether they just need to direct the tool how to deal with
nullvalues and let the tool generate the boilerplate to handle them. My preference would be the latter, so that if, in a future version of Dart, we get a better syntax for dealing with this kind of situation, we can change the tool and transform authors don't have to update their transforms.LongCatIsLooong commentedon Jun 11, 2023
Yeah that works for my use case. Out of curiosity, would it be as easy to support if the argument's type is a sealed class instead of
double?bwilkerson commentedon Jun 12, 2023
I don't think
sealedwould be a factor, but I might be missing something.Let me explain my thoughts more to see whether my conclusion makes sense. I'm guessing that the transform would have a variable like
scaleFactorwhose value is a fragment representing the value of the argument namedtextScaleFactorand then we'd allow a condition of the formscaleFactor.type == 'double'. That would kind of work, but relies on the textual representation of a type being unique, which isn't a valid assumption. (It's probably fine fordoublebecause very few developers are likely to define a type with that name, but there have been plenty of other name clashes in code.) In order to do better we'd have to know the scope in which to look up the name, and that gets hard. Having the class besealedwouldn't really make the lookup any easier.LongCatIsLooong commentedon Jun 13, 2023
Ah thanks for the explanation. What I meant was if one day someone would like to write a transform that migrate an API from a sealed class to something else, they would have to do this:
switch(fragment) { Square(..) =>, Circle(...), }, and that's a bit too verbose if the static type of the fragment isSquare?bwilkerson commentedon Jun 13, 2023
I agree; I wouldn't want to see the longer form when it isn't necessary (and it would probably produce code with warnings because some of the branches are dead code). It's just a question of how feasible it is to support this kind of change.
I think there will always be transforms that can't reasonably be encoded in the
fix_data.yamlfile because of their complexity. I also think that it's perfectly reasonable for us to use a more appropriate mechanism to provide those fixes when we need to do so.Replaces `textScaleFactor` with `TextScaler` (#128522)
Replaces `textScaleFactor` with `TextScaler` (flutter#128522)
Replaces `textScaleFactor` with `TextScaler` (#128522)