Re-land #52859: Revamp the engine style guide, remove always_specify_types
#53223
Conversation
matanlurey
added
the
autosubmit
CONTRIBUTING.md
Outdated
|
|
||
| ```dart | ||
| // Without 'num', the map would be inferred as 'Map<String, int>'. | ||
| const Map<String, num> map = {'one': 1, 'two': 2, 'three': 3}; |
There was a problem hiding this comment.
I think you mentioned you wanted to move the types to the right hand side here in this example? i.e.
const map = <String, num>{'one': 1, 'two': 2, 'three': 3};
| ```dart | ||
| // When variables are in the same scope, reduce() clearly returns an int. | ||
| var list = [1, 2, 3]; | ||
| var sum = list.reduce((a, b) => a + b); |
There was a problem hiding this comment.
In this example, I think @stuartmorgan had feedback at the meeting that the judgment of how much context is needed should be made on individual lines in isolation. That way, if in a subsequent change, the lines of code end up getting inserted between these two lines, we don't end up with a line who is too separated from that context. I think that's a pretty reasonable guideline personally, so I would be in favor of changing this example and perhaps even explicitly mentioning the rule of thumb Stuart suggested.
There was a problem hiding this comment.
I think we can rely on reviewers and drive-by code improvements to enforce these more subtle issues?
There was a problem hiding this comment.
Yeah to be clear, I hear the issue, I think it's worth thinking about. I don't think there is a specific line count I can suggest, some folks will think 10 lines are OK, others will balk at 4 lines.
Let's try this as-is, and if there are real issues that result of not having stricter guidance we can revisit.
There was a problem hiding this comment.
I double-checked the C++ guide later, and I was wrong about auto guidance being line-level. It just says (for local variables) to only use it "to make the code clearer by eliminating type information that is obvious or irrelevant". What is "obvious" is entirely left to discretion.
matanlurey
removed
the
autosubmit
matanlurey
added
the
autosubmit
…e `always_specify_types` (flutter/engine#53223)
…e `always_specify_types` (flutter/engine#53223)
…e `always_specify_types` (flutter/engine#53223)
…e `always_specify_types` (flutter/engine#53223)
…149770) flutter/engine@11a32d4...f377330 2024-06-05 skia-flutter-autoroll@skia.org Roll Skia from 37755d48cca3 to 8448abc95867 (7 revisions) (flutter/engine#53230) 2024-06-05 skia-flutter-autoroll@skia.org Roll Dart SDK from dac7c04c7342 to f838a9a8d45f (3 revisions) (flutter/engine#53225) 2024-06-05 skia-flutter-autoroll@skia.org Roll Skia from 337c3c4d1f1b to 37755d48cca3 (45 revisions) (flutter/engine#53224) 2024-06-05 yjbanov@google.com [web] enable always_specify_types for web_ui (flutter/engine#53226) 2024-06-05 matanlurey@users.noreply.github.com Re-land #52859: Revamp the engine style guide, remove `always_specify_types` (flutter/engine#53223) 2024-06-05 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from pagJoGS4kQ8Efa_if... to 2xWubo2mRP_2_wXKJ... (flutter/engine#53222) 2024-06-05 fmil@google.com [icu] Manual roll of icu (flutter/engine#53199) 2024-06-05 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[ios][platform_view] Use CAShapeLayer as the mask to avoid software rendering (#53072)" (flutter/engine#53220) 2024-06-05 41930132+hellohuanlin@users.noreply.github.com [ios][platform_view] Use CAShapeLayer as the mask to avoid software rendering (flutter/engine#53072) 2024-06-05 skia-flutter-autoroll@skia.org Roll Dart SDK from 343c20614708 to dac7c04c7342 (1 revision) (flutter/engine#53209) 2024-06-04 jonahwilliams@google.com [Impeller] Intel iOS Simulators must block on GPU completion. (flutter/engine#53073) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from pagJoGS4kQ8E to 2xWubo2mRP_2 If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC matanl@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
| // Literals (strings, numbers, lists, maps, etc) always return the same type. | ||
| var name = 'John Doe'; | ||
| var flag = true; | ||
| var numbers = [1, 2, 3]; |
There was a problem hiding this comment.
(this PR was posted and landed while I was out of town so I didn't have the opportunity to comment on it, so apologies for commenting after it landed)
This particular case ([1, 2, 3]) is one where having types explicitly somewhere (whether lhs or rhs doesn't matter) is actually really important. Consider for example an array [1.1] where one day the 1.1 is changed to 1. Despite this looking like an innocent change, it will change the inferred type.
In general, the int and double literals are not uniquely typed, and so I would always specify the types when number literals are used.
It's especially important because there's actually three types. For example, [1.1, 2] is not a List<double>, it's a List<num>. It's very easy to end up in situations where the exact type is not actually what it seems to "obviously" be, especially when code is being edited (as opposed to first written).
There was a problem hiding this comment.
Are there real-world cases where that change in inferred type both matters, but isn't caught by the compiler?
There was a problem hiding this comment.
One of the most common places for array literals is tests, where often the literals are then compared in type-unsafe ways (e.g. using expect). Not only would the type be not what the developer expects, and not only would the compiler not notice, but the test might even still pass despite the type being "wrong". This can lead to different people reading the code getting different interpretations of what the code intended (especially after multiple people have edited the code), which is exactly the thing we're trying to avoid by having a consistent style.
Explicit types everywhere avoids that in a verbose and brute-force manner. The more nuanced style suggestions here are much less verbose and lead to leaner code, but at the cost of requiring more thought when deciding where to include types or not include types. One example of that is this int/num/double confusion.
| var name = 'John Doe'; | ||
| var flag = true; | ||
| var numbers = [1, 2, 3]; | ||
| var map = {'one': 1, 'two': 2, 'three': 3}; |
There was a problem hiding this comment.
this is a similar example. Consider {'one': 1.0, 'two': 2.0, 'three': 3.0}. If you add 'four': 4 (which is a valid way of specifying a new value in a Map<String, double>, suddenly the type of the map changes (to Map<String, num>).
| var string = foo().toString(); | ||
|
|
||
| // It's reasonable to assume that length returns an int. | ||
| var length = string.length; |
There was a problem hiding this comment.
Whether it's reasonable or not seems pretty subjective. (Also, it's not actually reasonable for "String" to have a "length" member in the first place... what should it return? It's almost never what you want. list.length might be less controversial, though I would still give an explicit int type.)
|
|
||
| ```dart | ||
| // When variables are in the same scope, reduce() clearly returns an int. | ||
| var list = [1, 2, 3]; |
There was a problem hiding this comment.
as noted above, the type of this literal is not obvious.
| ImageBuffer buffer = fetchLatest(); | ||
|
|
||
| // What does this large chain of method calls return? | ||
| Iterable<int> numbers = foo().bar().map((b) => b.baz()); |
There was a problem hiding this comment.
for these in general I would recommend putting the types on the method calls, so that the type checking happens earlier in the process (e.g. using map<int>). It also avoids having the long Iterable<int> name.
|
|
||
| ```dart | ||
| // Without 'num', the map would be inferred as 'Map<String, int>'. | ||
| const map = <String, num>{'one': 1, 'two': 2, 'three': 3}; |
There was a problem hiding this comment.
Ideally we'd never do anything with explicit num types, it has performance implications. I think this example would be just as valid but more realistic if instead of num we said double.
Based on the (internal) discussion around converging on using the official Dart style guide, the design discussion https://flutter.dev/go/use-dart-style-in-flutter-engine, and with the exception of the code that gets published under
dart:ui(as that is user-facing, and we'd like to evolve the code style in conjunction with the framework), we're going to be (gradually) adopting the Dart style guide influtter/engine.For now, we:
always_specify_types(except forlib/ui, i.e.dart:ui)type_annotate_public_apis(which is now relevant since the above is disabled)I also took the opportunity to specify more about our style guide use in general, mostly to make it easier to understand our conventions, and also call out known problem areas (notably, our over-use of
shared_ptrandautoin some cases). I am happy to split those up, but it was easier to make the markdown changes at once.I also took @cbracken and folks advice and clarified directly that explicit types in Dart are not bad (with examples).