From d3053901d064877691e6dba48f05d1992179a7f5 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Wed, 5 Jun 2024 08:58:43 -0700 Subject: [PATCH 1/2] Re-land #52859. --- CONTRIBUTING.md | 187 +++++++++++++++++++++++++++++++---- analysis_options.yaml | 4 +- lib/ui/analysis_options.yaml | 1 + 3 files changed, 173 insertions(+), 19 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 07b35b3eb5630..718e666663229 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -14,29 +14,182 @@ contributing guide. ## Style -The Flutter engine follows Google style for the languages it uses: - -- [C++](https://google.github.io/styleguide/cppguide.html) - - **Note**: The Linux embedding generally follows idiomatic GObject-based C - style. Use of C++ is discouraged in that embedding to avoid creating hybrid - code that feels unfamiliar to either developers used to working with - `GObject` or C++ developers. For example, do not use STL collections or - `std::string`. Exceptions: - - C-style casts are forbidden; use C++ casts. - - Use `nullptr` rather than `NULL`. - - Avoid `#define`; for internal constants use `static constexpr` instead. -- [Objective-C][objc_style] (including [Objective-C++][objcc_style]) -- [Java][java_style] - -C/C++ and Objective-C/C++ files are formatted with `clang-format`, and GN files -with `gn format`. +The Flutter engine _generally_ follows Google style for the languages it uses, +with some exceptions. +### C/C++ + +Follows the [Google C++ Style Guide][google_cpp_style] and is automatically +formatted with `clang-format`. + +Some additional considerations that are in compliance with the style guide, but +are worth noting: + +#### Judiciously use shared_ptr + +The engine currently (as of 2024-05-15) uses `shared_ptr` liberally, which can +be expensive to copy, and is not always necessary. + +The C++ style guide has a +[section on ownership and smart pointers][cpp_ownership] worth reading: + +> Do not design your code to use shared ownership without a very good reason. +> One such reason is to avoid expensive copy operations, but you should only do +> this if the performance benefits are significant, and the underlying object is +> immutable. + +Prefer using `std::unique_ptr` when possible. + +#### Judiciously use `auto` + +The C++ style guide has a [section on type deduction][cpp_auto] that is worth +reading: + +> The fundamental rule is: use type deduction only to make the code clearer or +> safer, and do not use it merely to avoid the inconvenience of writing an +> explicit type. When judging whether the code is clearer, keep in mind that +> your readers are not necessarily on your team, or familiar with your project, +> so types that you and your reviewer experience as unnecessary clutter will +> very often provide useful information to others. For example, you can assume +> that the return type of `make_unique()` is obvious, but the return type +> of `MyWidgetFactory()` probably isn't. + +Due to our codebase's extensive use of `shared_ptr`, `auto` can have surprising +performance implications. See [#49801][pr_49801] for an example. + +#### Linux Embedding + +> [!NOTE] +> The Linux embedding instead follows idiomatic GObject-based C style. + +Use of C++ in the [Linux embedding][] is discouraged in that embedding to avoid +creating hybrid code that feels unfamiliar to either developers used to working +with `GObject` or C++ developers. + +For example, _do not_ use STL collections or `std::string`, but _do_: + +- Use C++ casts (C-style casts are forbidden). +- Use `nullptr` rather than `NULL`. +- Avoid `#define`; for internal constants use `static constexpr` instead. + +### Dart + +The Flutter engine _intends_ to follow the [Dart style guide][dart_style] but +currently follows the [Flutter style guide][flutter_style], with the following +exceptions: + +#### Use of type inference is allowed + +The [Dart style guide][dart_inference] only requires explicit types when type +inference is not possible, but the Flutter style guide always requires explicit +types. The engine is moving towards the Dart style guide, but this is a gradual +process. In the meantime, follow these guidelines: + +- **Always** annotate when inference is not possible. +- **Prefer** annotating when inference is possible but the type is not + obvious. + +Some cases when using `var`/`final`/`const` is appropriate: + +- When the type is obvious from the right-hand side of the assignment: + + ```dart + // Capitalized constructor name always returns a Foo. + var foo = Foo(); + + // Similar with factory constructors. + var bar = Bar.create(); + + // Literals (strings, numbers, lists, maps, etc) always return the same type. + var name = 'John Doe'; + var flag = true; + var numbers = [1, 2, 3]; + var map = {'one': 1, 'two': 2, 'three': 3}; + ``` + +- When the type is obvious from the method name: + + ```dart + // toString() always returns a String. + var string = foo().toString(); + + // It's reasonable to assume that length returns an int. + var length = string.length; + ``` + +- When the type is obvious from the context: + + ```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); + ``` + +Some cases where an explicit type should be considered: + +- When the type is not obvious from the right-hand side of the assignment: + + ```dart + // What does 'fetchLatest()' return? + ImageBuffer buffer = fetchLatest(); + + // What does this large chain of method calls return? + Iterable numbers = foo().bar().map((b) => b.baz()); + ``` + +- When there are semantic implications to the type: + + ```dart + // Without 'num', the map would be inferred as 'Map'. + const Map map = {'one': 1, 'two': 2, 'three': 3}; + ``` + +- Or, **when a reviewer requests it**! + + Remember that the goal is to make the code more readable and maintainable, and + explicit types _can_ help with that. Code can be changed, so it's always + possible to add or remove type annotations later as the code evolves, so avoid + bikeshedding over this. + +### Java + +Follows the [Google Java Style Guide][java_style] and is automatically formatted +with `google-java-format`. + +### Objective-C + +Follows the [Google Objective-C Style Guide][objc_style], including for +Objective-C++ and is automatically formatted with `clang-format`. + +### Python + +Follows the [Google Python Style Guide][google_python_style] and is +automatically formatted with `yapf`. + +> [!WARNING] +> Historically, the engine grew a number of one-off Python scripts, often as +> part of the testing or build infrastructure (i.e. command-line tools). We are +> instead moving towards using Dart for these tasks, so new Python scripts +> should be avoided whenever possible. + +### GN + +Automatically formatted with `gn format`. + +[cpp_auto]: https://google.github.io/styleguide/cppguide.html#Type_deduction +[cpp_ownership]: https://google.github.io/styleguide/cppguide.html#Ownership_and_Smart_Pointers +[dart_inference]: https://dart.dev/effective-dart/design#types +[dart_style]: https://dart.dev/effective-dart/style +[linux embedding]: shell/platform/linux +[google_cpp_style]: https://google.github.io/styleguide/cppguide.html +[pr_49801]: https://github.com/flutter/engine/pull/49801 [code_of_conduct]: https://github.com/flutter/flutter/blob/master/CODE_OF_CONDUCT.md [contrib_guide]: https://github.com/flutter/flutter/blob/master/CONTRIBUTING.md [engine_dev_setup]: https://github.com/flutter/flutter/blob/master/docs/engine/contributing/Setting-up-the-Engine-development-environment.md +[flutter_style]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [objc_style]: https://google.github.io/styleguide/objcguide.html -[objcc_style]: https://google.github.io/styleguide/objcguide.html#objective-c [java_style]: https://google.github.io/styleguide/javaguide.html +[google_python_style]: https://google.github.io/styleguide/pyguide.html ## Testing diff --git a/analysis_options.yaml b/analysis_options.yaml index 6462a509aeed8..ddd52e0f683a3 100644 --- a/analysis_options.yaml +++ b/analysis_options.yaml @@ -29,7 +29,7 @@ linter: - always_declare_return_types - always_put_control_body_on_new_line # - always_put_required_named_parameters_first # we prefer having parameters in the same order as fields https://github.com/flutter/flutter/issues/10219 - - always_specify_types + # - always_specify_types # DIFFERENT FROM FLUTTER/FLUTTER; see https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#dart # - always_use_package_imports # we do this commonly - annotate_overrides # - avoid_annotating_with_dynamic # conflicts with always_specify_types @@ -191,7 +191,7 @@ linter: - test_types_in_equals - throw_in_finally - tighten_type_of_initializing_formals - # - type_annotate_public_apis # subset of always_specify_types + - type_annotate_public_apis # DIFFERENT FROM FLUTTER/FLUTTER; this repo disable always_specify_types (https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#dart) - type_init_formals - type_literal_in_constant_pattern # - unawaited_futures # too many false positives, especially with the way AnimationController works diff --git a/lib/ui/analysis_options.yaml b/lib/ui/analysis_options.yaml index e84ed2920e8f6..d46ea0e5ca6a4 100644 --- a/lib/ui/analysis_options.yaml +++ b/lib/ui/analysis_options.yaml @@ -7,4 +7,5 @@ analyzer: linter: rules: + always_specify_types: true # dart:ui is shipped as part of flutter/flutter, let's keep them consistent for now unreachable_from_main: false # lint not compatible with how dart:ui is structured From 944b50e6b9bb7d7ca581cafb77ec5f5ff69ab9e4 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Wed, 5 Jun 2024 09:19:32 -0700 Subject: [PATCH 2/2] ++ --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 718e666663229..a2d228c3c0115 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -141,7 +141,7 @@ Some cases where an explicit type should be considered: ```dart // Without 'num', the map would be inferred as 'Map'. - const Map map = {'one': 1, 'two': 2, 'three': 3}; + const map = {'one': 1, 'two': 2, 'three': 3}; ``` - Or, **when a reviewer requests it**!