diff options
Diffstat (limited to 'docs/code-quality')
117 files changed, 8746 insertions, 0 deletions
diff --git a/docs/code-quality/coding-style/about-logins-rtl.png b/docs/code-quality/coding-style/about-logins-rtl.png Binary files differnew file mode 100644 index 0000000000..a5d0edd4c8 --- /dev/null +++ b/docs/code-quality/coding-style/about-logins-rtl.png diff --git a/docs/code-quality/coding-style/about-protections-rtl.png b/docs/code-quality/coding-style/about-protections-rtl.png Binary files differnew file mode 100644 index 0000000000..4fbbf5e889 --- /dev/null +++ b/docs/code-quality/coding-style/about-protections-rtl.png diff --git a/docs/code-quality/coding-style/coding_style_cpp.rst b/docs/code-quality/coding-style/coding_style_cpp.rst new file mode 100644 index 0000000000..cb4764cea5 --- /dev/null +++ b/docs/code-quality/coding-style/coding_style_cpp.rst @@ -0,0 +1,1150 @@ +================ +C++ Coding style +================ + + +This document attempts to explain the basic styles and patterns used in +the Mozilla codebase. New code should try to conform to these standards, +so it is as easy to maintain as existing code. There are exceptions, but +it's still important to know the rules! + +This article is particularly for those new to the Mozilla codebase, and +in the process of getting their code reviewed. Before requesting a +review, please read over this document, making sure that your code +conforms to recommendations. + +.. container:: blockIndicator warning + + The Firefox code base adopts parts of the `Google Coding style for C++ + code <https://google.github.io/styleguide/cppguide.html>`__, but not all of its rules. + A few rules are followed across the code base, others are intended to be + followed in new or significantly revised code. We may extend this list in the + future, when we evaluate the Google Coding Style for C++ Code further and/or update + our coding practices. However, the plan is not to adopt all rules of the Google Coding + Style for C++ Code. Some rules are explicitly unlikely to be adopted at any time. + + Followed across the code base: + + - `Formatting <https://google.github.io/styleguide/cppguide.html#Formatting>`__, + except for subsections noted here otherwise + - `Implicit Conversions <https://google.github.io/styleguide/cppguide.html#Implicit_Conversions>`__, + which is enforced by a custom clang-plugin check, unless explicitly overridden using + ``MOZ_IMPLICIT`` + + Followed in new/significantly revised code: + + - `Include guards <https://google.github.io/styleguide/cppguide.html#The__define_Guard>`__ + + Unlikely to be ever adopted: + + - `Forward declarations <https://google.github.io/styleguide/cppguide.html#Forward_Declarations>`__ + - `Formatting/Conditionals <https://google.github.io/styleguide/cppguide.html#Conditionals>`__ + w.r.t. curly braces around inner statements, we require them in all cases where the + Google style allows to leave them out for single-line conditional statements + + This list reflects the state of the Google Google Coding Style for C++ Code as of + 2020-07-17. It may become invalid when the Google modifies its Coding Style. + + +Formatting code +--------------- + +Formatting is done automatically via clang-format, and controlled via in-tree +configuration files. See :ref:`Formatting C++ Code With clang-format` +for more information. + +Unix-style linebreaks (``\n``), not Windows-style (``\r\n``). You can +convert patches, with DOS newlines to Unix via the ``dos2unix`` utility, +or your favorite text editor. + +Static analysis +--------------- + +Several of the rules in the Google C++ coding styles and the additions mentioned below +can be checked via clang-tidy (some rules are from the upstream clang-tidy, some are +provided via a mozilla-specific plugin). Some of these checks also allow fixes to +be automatically applied. + +``mach static-analysis`` provides a convenient way to run these checks. For example, +for the check called ``google-readability-braces-around-statements``, you can run: + +.. code-block:: shell + + ./mach static-analysis check --checks="-*,google-readability-braces-around-statements" --fix <file> + +It may be necessary to reformat the files after automatically applying fixes, see +:ref:`Formatting C++ Code With clang-format`. + +Additional rules +---------------- + +*The norms in this section should be followed for new code. For existing code, +use the prevailing style in a file or module, ask the owner if you are +in another team's codebase or it's not clear what style to use.* + + + + +Control structures +~~~~~~~~~~~~~~~~~~ + +Always brace controlled statements, even a single-line consequent of +``if else else``. This is redundant, typically, but it avoids dangling +else bugs, so it's safer at scale than fine-tuning. + +Examples: + +.. code-block:: cpp + + if (...) { + } else if (...) { + } else { + } + + while (...) { + } + + do { + } while (...); + + for (...; ...; ...) { + } + + switch (...) { + case 1: { + // When you need to declare a variable in a switch, put the block in braces. + int var; + break; + } + case 2: + ... + break; + default: + break; + } + +``else`` should only ever be followed by ``{`` or ``if``; i.e., other +control keywords are not allowed and should be placed inside braces. + +.. note:: + + For this rule, clang-tidy provides the ``google-readability-braces-around-statements`` + check with autofixes. + + +C++ namespaces +~~~~~~~~~~~~~~ + +Mozilla project C++ declarations should be in the ``mozilla`` +namespace. Modules should avoid adding nested namespaces under +``mozilla``, unless they are meant to contain names which have a high +probability of colliding with other names in the code base. For example, +``Point``, ``Path``, etc. Such symbols can be put under +module-specific namespaces, under ``mozilla``, with short +all-lowercase names. Other global namespaces besides ``mozilla`` are +not allowed. + +No ``using`` directives are allowed in header files, except inside class +definitions or functions. (We don't want to pollute the global scope of +compilation units that use the header file.) + +.. note:: + + For parts of this rule, clang-tidy provides the ``google-global-names-in-headers`` + check. It only detects ``using namespace`` directives in the global namespace. + + +``using namespace ...;`` is only allowed in ``.cpp`` files after all +``#include``\ s. Prefer to wrap code in ``namespace ... { ... };`` +instead, if possible. ``using namespace ...;``\ should always specify +the fully qualified namespace. That is, to use ``Foo::Bar`` do not +write ``using namespace Foo; using namespace Bar;``, write +``using namespace Foo::Bar;`` + +Use nested namespaces (ex: ``namespace mozilla::widget {`` + +.. note:: + + clang-tidy provides the ``modernize-concat-nested-namespaces`` + check with autofixes. + + +Anonymous namespaces +~~~~~~~~~~~~~~~~~~~~ + +We prefer using ``static``, instead of anonymous C++ namespaces. This may +change once there is better debugger support (especially on Windows) for +placing breakpoints, etc. on code in anonymous namespaces. You may still +use anonymous namespaces for things that can't be hidden with ``static``, +such as types, or certain objects which need to be passed to template +functions. + + +C++ classes +~~~~~~~~~~~~ + +.. code-block:: cpp + + namespace mozilla { + + class MyClass : public A + { + ... + }; + + class MyClass + : public X + , public Y + { + public: + MyClass(int aVar, int aVar2) + : mVar(aVar) + , mVar2(aVar2) + { + ... + } + + // Special member functions, like constructors, that have default bodies + // should use '= default' annotation instead. + MyClass() = default; + + // Unless it's a copy or move constructor or you have a specific reason to allow + // implicit conversions, mark all single-argument constructors explicit. + explicit MyClass(OtherClass aArg) + { + ... + } + + // This constructor can also take a single argument, so it also needs to be marked + // explicit. + explicit MyClass(OtherClass aArg, AnotherClass aArg2 = AnotherClass()) + { + ... + } + + int LargerFunction() + { + ... + ... + } + + private: + int mVar; + }; + + } // namespace mozilla + +Define classes using the style given above. + +.. note:: + + For the rule on ``= default``, clang-tidy provides the ``modernize-use-default`` + check with autofixes. + + For the rule on explicit constructors and conversion operators, clang-tidy + provides the ``mozilla-implicit-constructor`` check. + +Existing classes in the global namespace are named with a short prefix +(For example, ``ns``) as a pseudo-namespace. + + +Methods and functions +~~~~~~~~~~~~~~~~~~~~~ + + +C/C++ +^^^^^ + +In C/C++, method names should use ``UpperCamelCase``. + +Getters that never fail, and never return null, are named ``Foo()``, +while all other getters use ``GetFoo()``. Getters can return an object +value, via a ``Foo** aResult`` outparam (typical for an XPCOM getter), +or as an ``already_AddRefed<Foo>`` (typical for a WebIDL getter, +possibly with an ``ErrorResult& rv`` parameter), or occasionally as a +``Foo*`` (typical for an internal getter for an object with a known +lifetime). See `the bug 223255 <https://bugzilla.mozilla.org/show_bug.cgi?id=223255>`_ +for more information. + +XPCOM getters always return primitive values via an outparam, while +other getters normally use a return value. + +Method declarations must use, at most, one of the following keywords: +``virtual``, ``override``, or ``final``. Use ``virtual`` to declare +virtual methods, which do not override a base class method with the same +signature. Use ``override`` to declare virtual methods which do +override a base class method, with the same signature, but can be +further overridden in derived classes. Use ``final`` to declare virtual +methods which do override a base class method, with the same signature, +but can NOT be further overridden in the derived classes. This should +help the person reading the code fully understand what the declaration +is doing, without needing to further examine base classes. + +.. note:: + + For the rule on ``virtual/override/final``, clang-tidy provides the + ``modernize-use-override`` check with autofixes. + + +Operators +~~~~~~~~~ + +The unary keyword operator ``sizeof``, should have its operand parenthesized +even if it is an expression; e.g. ``int8_t arr[64]; memset(arr, 42, sizeof(arr));``. + + +Literals +~~~~~~~~ + +Use ``\uXXXX`` unicode escapes for non-ASCII characters. The character +set for XUL, script, and properties files is UTF-8, which is not easily +readable. + + +Prefixes +~~~~~~~~ + +Follow these naming prefix conventions: + + +Variable prefixes +^^^^^^^^^^^^^^^^^ + +- k=constant (e.g. ``kNC_child``). Not all code uses this style; some + uses ``ALL_CAPS`` for constants. +- g=global (e.g. ``gPrefService``) +- a=argument (e.g. ``aCount``) +- C++ Specific Prefixes + + - s=static member (e.g. ``sPrefChecked``) + - m=member (e.g. ``mLength``) + - e=enum variants (e.g. ``enum Foo { eBar, eBaz }``). Enum classes + should use ``CamelCase`` instead (e.g. + ``enum class Foo { Bar, Baz }``). + + +Global functions/macros/etc +^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +- Macros begin with ``MOZ_``, and are all caps (e.g. + ``MOZ_WOW_GOODNESS``). Note that older code uses the ``NS_`` prefix; + while these aren't being changed, you should only use ``MOZ_`` for + new macros. The only exception is if you're creating a new macro, + which is part of a set of related macros still using the old ``NS_`` + prefix. Then you should be consistent with the existing macros. + + +Error Variables +^^^^^^^^^^^^^^^ + +- Local variables that are assigned ``nsresult`` result codes should be named ``rv`` + (i.e., e.g., not ``res``, not ``result``, not ``foo``). `rv` should not be + used for bool or other result types. +- Local variables that are assigned ``bool`` result codes should be named `ok`. + + +C/C++ practices +--------------- + +- **Have you checked for compiler warnings?** Warnings often point to + real bugs. `Many of them <https://searchfox.org/mozilla-central/source/build/moz.configure/warnings.configure>`__ + are enabled by default in the build system. +- In C++ code, use ``nullptr`` for pointers. In C code, using ``NULL`` + or ``0`` is allowed. + +.. note:: + + For the C++ rule, clang-tidy provides the ``modernize-use-nullptr`` check + with autofixes. + +- Don't use ``PRBool`` and ``PRPackedBool`` in C++, use ``bool`` + instead. +- For checking if a ``std`` container has no items, don't use + ``size()``, instead use ``empty()``. +- When testing a pointer, use ``(!myPtr)`` or ``(myPtr)``; + don't use ``myPtr != nullptr`` or ``myPtr == nullptr``. +- Do not compare ``x == true`` or ``x == false``. Use ``(x)`` or + ``(!x)`` instead. ``if (x == true)`` may have semantics different from + ``if (x)``! + +.. note:: + + clang-tidy provides the ``readability-simplify-boolean-expr`` check + with autofixes that checks for these and some other boolean expressions + that can be simplified. + +- In general, initialize variables with ``nsFoo aFoo = bFoo,`` and not + ``nsFoo aFoo(bFoo)``. + + - For constructors, initialize member variables with : ``nsFoo + aFoo(bFoo)`` syntax. + +- To avoid warnings created by variables used only in debug builds, use + the + `DebugOnly<T> <https://developer.mozilla.org/docs/Mozilla/Debugging/DebugOnly%3CT%3E>`__ + helper when declaring them. +- You should `use the static preference + API <https://firefox-source-docs.mozilla.org/modules/libpref/index.html>`__ for + working with preferences. +- One-argument constructors, that are not copy or move constructors, + should generally be marked explicit. Exceptions should be annotated + with ``MOZ_IMPLICIT``. +- Use ``char32_t`` as the return type or argument type of a method that + returns or takes as argument a single Unicode scalar value. (Don't + use UTF-32 strings, though.) +- Prefer unsigned types for semantically-non-negative integer values. +- When operating on integers that could overflow, use ``CheckedInt``. +- Avoid the usage of ``typedef``, instead, please use ``using`` instead. + +.. note:: + + For parts of this rule, clang-tidy provides the ``modernize-use-using`` + check with autofixes. + + +Header files +------------ + +Since the Firefox code base is huge and uses a monolithic build, it is +of utmost importance for keeping build times reasonable to limit the +number of included files in each translation unit to the required minimum. +Exported header files need particular attention in this regard, since their +included files propagate, and many of them are directly or indirectly +included in a large number of translation units. + +- Include guards are named per the Google coding style (i.e. upper snake + case with a single trailing underscore). They should not include a + leading ``MOZ_`` or ``MOZILLA_``. For example, ``dom/media/foo.h`` + would use the guard ``DOM_MEDIA_FOO_H_``. +- Forward-declare classes in your header files, instead of including + them, whenever possible. For example, if you have an interface with a + ``void DoSomething(nsIContent* aContent)`` function, forward-declare + with ``class nsIContent;`` instead of ``#include "nsIContent.h"``. + If a "forwarding header" is provided for a type, include that instead of + putting the literal forward declaration(s) in your header file. E.g. for + some JavaScript types, there is ``js/TypeDecls.h``, for the string types + there is ``StringFwd.h``. One reason for this is that this allows + changing a type to a type alias by only changing the forwarding header. + The following uses of a type can be done with a forward declaration only: + + - Parameter or return type in a function declaration + - Member/local variable pointer or reference type + - Use as a template argument (not in all cases) in a member/local variable type + - Defining a type alias + + The following uses of a type require a full definition: + + - Base class + - Member/local variable type + - Use with delete or new + - Use as a template argument (not in all cases) + - Any uses of non-scoped enum types + - Enum values of a scoped enum type + + Use as a template argument is somewhat tricky. It depends on how the + template uses the type. E.g. ``mozilla::Maybe<T>`` and ``AutoTArray<T>`` + always require a full definition of ``T`` because the size of the + template instance depends on the size of ``T``. ``RefPtr<T>`` and + ``UniquePtr<T>`` don't require a full definition (because their + pointer member always has the same size), but their destructor + requires a full definition. If you encounter a template that cannot + be instantiated with a forward declaration only, but it seems + it should be possible, please file a bug (if it doesn't exist yet). + + Therefore, also consider the following guidelines to allow using forward + declarations as widely as possible. +- Inline function bodies in header files often pull in a lot of additional + dependencies. Be mindful when adding or extending inline function bodies, + and consider moving the function body to the cpp file or to a separate + header file that is not included everywhere. Bug 1677553 intends to provide + a more specific guideline on this. +- Consider the use of the `Pimpl idiom <https://en.cppreference.com/w/cpp/language/pimpl>`__, + i.e. hide the actual implementation in a separate ``Impl`` class that is + defined in the implementation file and only expose a ``class Impl;`` forward + declaration and ``UniquePtr<Impl>`` member in the header file. +- Do not use non-scoped enum types. These cannot be forward-declared. Use + scoped enum types instead, and forward declare them when possible. +- Avoid nested types that need to be referenced from outside the class. + These cannot be forward declared. Place them in a namespace instead, maybe + in an extra inner namespace, and forward declare them where possible. +- Avoid mixing declarations with different sets of dependencies in a single + header file. This is generally advisable, but even more so when some of these + declarations are used by a subset of the translation units that include the + combined header file only. Consider such a badly mixed header file like: + + .. code-block:: cpp + + /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ + /* vim: set ts=8 sts=2 et sw=2 tw=80: */ + /* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at http://mozilla.org/MPL/2.0/. */ + + #ifndef BAD_MIXED_FILE_H_ + #define BAD_MIXED_FILE_H_ + + // Only this include is needed for the function declaration below. + #include "nsCOMPtr.h" + + // These includes are only needed for the class definition. + #include "nsIFile.h" + #include "mozilla/ComplexBaseClass.h" + + namespace mozilla { + + class WrappedFile : public nsIFile, ComplexBaseClass { + // ... class definition left out for clarity + }; + + // Assuming that most translation units that include this file only call + // the function, but don't need the class definition, this should be in a + // header file on its own in order to avoid pulling in the other + // dependencies everywhere. + nsCOMPtr<nsIFile> CreateDefaultWrappedFile(nsCOMPtr<nsIFile>&& aFileToWrap); + + } // namespace mozilla + + #endif // BAD_MIXED_FILE_H_ + + +An example header file based on these rules (with some extra comments): + +.. code-block:: cpp + + /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ + /* vim: set ts=8 sts=2 et sw=2 tw=80: */ + /* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at http://mozilla.org/MPL/2.0/. */ + + #ifndef DOM_BASE_FOO_H_ + #define DOM_BASE_FOO_H_ + + // Include guards should come at the very beginning and always use exactly + // the style above. Otherwise, compiler optimizations that avoid rescanning + // repeatedly included headers might not hit and cause excessive compile + // times. + + #include <cstdint> + #include "nsCOMPtr.h" // This is needed because we have a nsCOMPtr<T> data member. + + class nsIFile; // Used as a template argument only. + enum class nsresult : uint32_t; // Used as a parameter type only. + template <class T> + class RefPtr; // Used as a return type only. + + namespace mozilla::dom { + + class Document; // Used as a template argument only. + + // Scoped enum, not as a nested type, so it can be + // forward-declared elsewhere. + enum class FooKind { Small, Big }; + + class Foo { + public: + // Do not put the implementation in the header file, it would + // require including nsIFile.h + Foo(nsCOMPtr<nsIFile> aFile, FooKind aFooKind); + + RefPtr<Document> CreateDocument(); + + void SetResult(nsresult aResult); + + // Even though we will default this destructor, do this in the + // implementation file since we would otherwise need to include + // nsIFile.h in the header. + ~Foo(); + + private: + nsCOMPtr<nsIFile> mFile; + }; + + } // namespace mozilla::dom + + #endif // DOM_BASE_FOO_H_ + + +Corresponding implementation file: + +.. code-block:: cpp + + /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ + /* vim: set ts=8 sts=2 et sw=2 tw=80: */ + /* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at http://mozilla.org/MPL/2.0/. */ + + #include "mozilla/dom/Foo.h" // corresponding header + + #include "mozilla/Assertions.h" // Needed for MOZ_ASSERT. + #include "mozilla/dom/Document.h" // Needed because we construct a Document. + #include "nsError.h" // Needed because we use NS_OK aka nsresult::NS_OK. + #include "nsIFile.h" // This is needed because our destructor indirectly calls delete nsIFile in a template instance. + + namespace mozilla::dom { + + // Do not put the implementation in the header file, it would + // require including nsIFile.h + Foo::Foo(nsCOMPtr<nsIFile> aFile, FooKind aFooKind) + : mFile{std::move(aFile)} { + } + + RefPtr<Document> Foo::CreateDocument() { + return MakeRefPtr<Document>(); + } + + void Foo::SetResult(nsresult aResult) { + MOZ_ASSERT(aResult != NS_OK); + + // do something with aResult + } + + // Even though we default this destructor, do this in the + // implementation file since we would otherwise need to include + // nsIFile.h in the header. + Foo::~Foo() = default; + + } // namespace mozilla::dom + + +Include directives +------------------ + +- Ordering: + + - In an implementation file (cpp file), the very first include directive + should include the corresponding header file, followed by a blank line. + - Any conditional includes (depending on some ``#ifdef`` or similar) follow + after non-conditional includes. Don't mix them in. + - Don't place comments between non-conditional includes. + + Bug 1679522 addresses automating the ordering via clang-format, which + is going to enforce some stricter rules. Expect the includes to be reordered. + If you include third-party headers that are not self-contained, and therefore + need to be included in a particular order, enclose those (and only those) + between ``// clang-format off`` and ``// clang-format on``. This should not be + done for Mozilla headers, which should rather be made self-contained if they + are not. + +- Brackets vs. quotes: C/C++ standard library headers are included using + brackets (e.g. ``#include <cstdint>``), all other include directives use + (double) quotes (e.g. ``#include "mozilla/dom/Document.h``). +- Exported headers should always be included from their exported path, not + from their source path in the tree, even if available locally. E.g. always + do ``#include "mozilla/Vector.h"``, not ``#include "Vector.h"``, even + from within `mfbt`. +- Generally, you should include exactly those headers that are needed, not + more and not less. Unfortunately this is not easy to see. Maybe C++20 + modules will bring improvements to this, but it will take a long time + to be adopted. +- The basic rule is that if you literally use a symbol in your file that + is declared in a header A.h, include that header. In particular in header + files, check if a forward declaration or including a forwarding header is + sufficient, see section :ref:`Header files`. + + There are cases where this basic rule is not sufficient. Some cases where + you need to include additional headers are: + + - You reference a member of a type that is not literally mentioned in your + code, but, e.g. is the return type of a function you are calling. + + There are also cases where the basic rule leads to redundant includes. Note + that "redundant" here does not refer to "accidentally redundant" headers, + e.g. at the time of writing ``mozilla/dom/BodyUtil.h`` includes + ``mozilla/dom/FormData.h``, but it doesn't need to (it only needs a forward + declaration), so including ``mozilla/dom/FormData.h`` is "accidentally + redundant" when including ``mozilla/dom/BodyUtil.h``. The includes of + ``mozilla/dom/BodyUtil.h`` might change at any time, so if a file that + includes ``mozilla/dom/BodyUtil.h`` needs a full definition of + ``mozilla::dom::FormData``, it should includes ``mozilla/dom/FormData.h`` + itself. In fact, these "accidentally redundant" headers MUST be included. + Relying on accidentally redundant includes makes any change to a header + file extremely hard, in particular when considering that the set of + accidentally redundant includes differs between platforms. + But some cases in fact are non-accidentally redundant, and these can and + typically should not be repeated: + + - The includes of the header file do not need to be repeated in its + corresponding implementation file. Rationale: the implementation file and + its corresponding header file are tightly coupled per se. + + Macros are a special case. Generally, the literal rule also applies here, + i.e. if the macro definition references a symbol, the file containing the + macro definition should include the header defining the symbol. E.g. + ``NS_IMPL_CYCLE_COLLECTING_NATIVE_RELEASE`` defined in ``nsISupportsImpl.h`` + makes use of ``MOZ_ASSERT`` defined in ``mozilla/Assertions.h``, so + ``nsISupportsImpl.h`` includes ``mozilla/Assertions.h``. However, this + requires human judgment of what is intended, since technically only the + invocations of the macro reference a symbol (and that's how + include-what-you-use handles this). It might depend on the + context or parameters which symbol is actually referenced, and sometimes + this is on purpose. In these cases, the user of the macro needs to include + the required header(s). + + + +COM and pointers +---------------- + +- Use ``nsCOMPtr<>`` + If you don't know how to use it, start looking in the code for + examples. The general rule, is that the very act of typing + ``NS_RELEASE`` should be a signal to you to question your code: + "Should I be using ``nsCOMPtr`` here?". Generally the only valid use + of ``NS_RELEASE`` is when you are storing refcounted pointers in a + long-lived datastructure. +- Declare new XPCOM interfaces using :doc:`XPIDL </xpcom/xpidl>`, so they + will be scriptable. +- Use :doc:`nsCOMPtr </xpcom/refptr>` for strong references, and + ``nsWeakPtr`` for weak references. +- Don't use ``QueryInterface`` directly. Use ``CallQueryInterface`` or + ``do_QueryInterface`` instead. +- Use :ref:`Contract IDs <contract_ids>`, + instead of CIDs with ``do_CreateInstance``/``do_GetService``. +- Use pointers, instead of references for function out parameters, even + for primitive types. + + +IDL +--- + + +Use leading-lowercase, or "interCaps" +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +When defining a method or attribute in IDL, the first letter should be +lowercase, and each following word should be capitalized. For example: + +.. code-block:: cpp + + long updateStatusBar(); + + +Use attributes wherever possible +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Whenever you are retrieving or setting a single value, without any +context, you should use attributes. Don't use two methods when you could +use an attribute. Using attributes logically connects the getting and +setting of a value, and makes scripted code look cleaner. + +This example has too many methods: + +.. code-block:: cpp + + interface nsIFoo : nsISupports + { + long getLength(); + void setLength(in long length); + long getColor(); + }; + +The code below will generate the exact same C++ signature, but is more +script-friendly. + +.. code-block:: cpp + + interface nsIFoo : nsISupports + { + attribute long length; + readonly attribute long color; + }; + + +Use Java-style constants +~~~~~~~~~~~~~~~~~~~~~~~~ + +When defining scriptable constants in IDL, the name should be all +uppercase, with underscores between words: + +.. code-block:: cpp + + const long ERROR_UNDEFINED_VARIABLE = 1; + + +See also +~~~~~~~~ + +For details on interface development, as well as more detailed style +guides, see the `Interface development +guide <https://developer.mozilla.org/docs/Mozilla/Developer_guide/Interface_development_guide>`__. + + +Error handling +-------------- + + +Check for errors early and often +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Every time you make a call into an XPCOM function, you should check for +an error condition. You need to do this even if you know that call will +never fail. Why? + +- Someone may change the callee in the future to return a failure + condition. +- The object in question may live on another thread, another process, + or possibly even another machine. The proxy could have failed to make + your call in the first place. + +Also, when you make a new function which is failable (i.e. it will +return a ``nsresult`` or a ``bool`` that may indicate an error), you should +explicitly mark the return value should always be checked. For example: + +:: + + // for IDL. + [must_use] nsISupports + create(); + + // for C++, add this in *declaration*, do not add it again in implementation. + [[nodiscard]] nsresult + DoSomething(); + +There are some exceptions: + +- Predicates or getters, which return ``bool`` or ``nsresult``. +- IPC method implementation (For example, ``bool RecvSomeMessage()``). +- Most callers will check the output parameter, see below. + +.. code-block:: cpp + + nsresult + SomeMap::GetValue(const nsString& key, nsString& value); + +If most callers need to check the output value first, then adding +``[[nodiscard]]`` might be too verbose. In this case, change the return value +to void might be a reasonable choice. + +There is also a static analysis attribute ``[[nodiscard]]``, which can +be added to class declarations, to ensure that those declarations are +always used when they are returned. + + +Use the NS_WARN_IF macro when errors are unexpected. +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +The ``NS_WARN_IF`` macro can be used to issue a console warning, in debug +builds if the condition fails. This should only be used when the failure +is unexpected and cannot be caused by normal web content. + +If you are writing code which wants to issue warnings when methods fail, +please either use ``NS_WARNING`` directly, or use the new ``NS_WARN_IF`` macro. + +.. code-block:: cpp + + if (NS_WARN_IF(somethingthatshouldbefalse)) { + return NS_ERROR_INVALID_ARG; + } + + if (NS_WARN_IF(NS_FAILED(rv))) { + return rv; + } + +Previously, the ``NS_ENSURE_*`` macros were used for this purpose, but +those macros hide return statements, and should not be used in new code. +(This coding style rule isn't generally agreed, so use of ``NS_ENSURE_*`` +can be valid.) + + +Return from errors immediately +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +In most cases, your knee-jerk reaction should be to return from the +current function, when an error condition occurs. Don't do this: + +.. code-block:: cpp + + rv = foo->Call1(); + if (NS_SUCCEEDED(rv)) { + rv = foo->Call2(); + if (NS_SUCCEEDED(rv)) { + rv = foo->Call3(); + } + } + return rv; + +Instead, do this: + +.. code-block:: cpp + + rv = foo->Call1(); + if (NS_FAILED(rv)) { + return rv; + } + + rv = foo->Call2(); + if (NS_FAILED(rv)) { + return rv; + } + + rv = foo->Call3(); + if (NS_FAILED(rv)) { + return rv; + } + +Why? Error handling should not obfuscate the logic of the code. The +author's intent, in the first example, was to make 3 calls in +succession. Wrapping the calls in nested if() statements, instead +obscured the most likely behavior of the code. + +Consider a more complicated example to hide a bug: + +.. code-block:: cpp + + bool val; + rv = foo->GetBooleanValue(&val); + if (NS_SUCCEEDED(rv) && val) { + foo->Call1(); + } else { + foo->Call2(); + } + +The intent of the author, may have been, that ``foo->Call2()`` would only +happen when val had a false value. In fact, ``foo->Call2()`` will also be +called, when ``foo->GetBooleanValue(&val)`` fails. This may, or may not, +have been the author's intent. It is not clear from this code. Here is +an updated version: + +.. code-block:: cpp + + bool val; + rv = foo->GetBooleanValue(&val); + if (NS_FAILED(rv)) { + return rv; + } + if (val) { + foo->Call1(); + } else { + foo->Call2(); + } + +In this example, the author's intent is clear, and an error condition +avoids both calls to ``foo->Call1()`` and ``foo->Call2();`` + +*Possible exceptions:* Sometimes it is not fatal if a call fails. For +instance, if you are notifying a series of observers that an event has +fired, it might be trivial that one of these notifications failed: + +.. code-block:: cpp + + for (size_t i = 0; i < length; ++i) { + // we don't care if any individual observer fails + observers[i]->Observe(foo, bar, baz); + } + +Another possibility, is you are not sure if a component exists or is +installed, and you wish to continue normally, if the component is not +found. + +.. code-block:: cpp + + nsCOMPtr<nsIMyService> service = do_CreateInstance(NS_MYSERVICE_CID, &rv); + // if the service is installed, then we'll use it. + if (NS_SUCCEEDED(rv)) { + // non-fatal if this fails too, ignore this error. + service->DoSomething(); + + // this is important, handle this error! + rv = service->DoSomethingImportant(); + if (NS_FAILED(rv)) { + return rv; + } + } + + // continue normally whether or not the service exists. + + +Strings +------- + +.. note:: + + This section overlaps with the more verbose advice given in + :doc:`String guide </xpcom/stringguide>`. + These should eventually be merged. For now, please refer to that guide for + more advice. + +- String arguments to functions should be declared as ``[const] nsA[C]String&``. +- Prefer using string literals. In particular, use empty string literals, + i.e. ``u""_ns`` or ``""_ns``, instead of ``Empty[C]String()`` or + ``const nsAuto[C]String empty;``. Use ``Empty[C]String()`` only if you + specifically need a ``const ns[C]String&``, e.g. with the ternary operator + or when you need to return/bind to a reference or take the address of the + empty string. +- For 16-bit literal strings, use ``u"..."_ns`` or, if necessary + ``NS_LITERAL_STRING_FROM_CSTRING(...)`` instead of ``nsAutoString()`` + or other ways that would do a run-time conversion. + See :ref:`Avoid runtime conversion of string literals <Avoid runtime conversion of string literals>` below. +- To compare a string with a literal, use ``.EqualsLiteral("...")``. +- Use ``str.IsEmpty()`` instead of ``str.Length() == 0``. +- Use ``str.Truncate()`` instead of ``str.SetLength(0)``, + ``str.Assign(""_ns)`` or ``str.AssignLiteral("")``. +- Don't use functions from ``ctype.h`` (``isdigit()``, ``isalpha()``, + etc.) or from ``strings.h`` (``strcasecmp()``, ``strncasecmp()``). + These are locale-sensitive, which makes them inappropriate for + processing protocol text. At the same time, they are too limited to + work properly for processing natural-language text. Use the + alternatives in ``mozilla/TextUtils.h`` and in ``nsUnicharUtils.h`` + in place of ``ctype.h``. In place of ``strings.h``, prefer the + ``nsStringComparator`` facilities for comparing strings or if you + have to work with zero-terminated strings, use ``nsCRT.h`` for + ASCII-case-insensitive comparison. + + +Use the ``Auto`` form of strings for local values +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +When declaring a local, short-lived ``nsString`` class, always use +``nsAutoString`` or ``nsAutoCString``. These pre-allocate a 64-byte +buffer on the stack, and avoid fragmenting the heap. Don't do this: + +.. code-block:: cpp + + nsresult + foo() + { + nsCString bar; + .. + } + +instead: + +.. code-block:: cpp + + nsresult + foo() + { + nsAutoCString bar; + .. + } + + +Be wary of leaking values from non-XPCOM functions that return char\* or PRUnichar\* +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +It is an easy trap to return an allocated string, from an internal +helper function, and then using that function inline in your code, +without freeing the value. Consider this code: + +.. code-block:: cpp + + static char* + GetStringValue() + { + .. + return resultString.ToNewCString(); + } + + .. + WarnUser(GetStringValue()); + +In the above example, ``WarnUser`` will get the string allocated from +``resultString.ToNewCString()`` and throw away the pointer. The +resulting value is never freed. Instead, either use the string classes, +to make sure your string is automatically freed when it goes out of +scope, or make sure that your string is freed. + +Automatic cleanup: + +.. code-block:: cpp + + static void + GetStringValue(nsAWritableCString& aResult) + { + .. + aResult.Assign("resulting string"); + } + + .. + nsAutoCString warning; + GetStringValue(warning); + WarnUser(warning.get()); + +Free the string manually: + +.. code-block:: cpp + + static char* + GetStringValue() + { + .. + return resultString.ToNewCString(); + } + + .. + char* warning = GetStringValue(); + WarnUser(warning); + nsMemory::Free(warning); + +.. _Avoid runtime conversion of string literals: + +Avoid runtime conversion of string literals +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +It is very common to need to assign the value of a literal string, such +as ``"Some String"``, into a unicode buffer. Instead of using ``nsString``'s +``AssignLiteral`` and ``AppendLiteral``, use a user-defined literal like `u"foo"_ns` +instead. On most platforms, this will force the compiler to compile in a +raw unicode string, and assign it directly. In cases where the literal is defined +via a macro that is used in both 8-bit and 16-bit ways, you can use +`NS_LITERAL_STRING_FROM_CSTRING` to do the conversion at compile time. + +Incorrect: + +.. code-block:: cpp + + nsAutoString warning; + warning.AssignLiteral("danger will robinson!"); + ... + foo->SetStringValue(warning); + ... + bar->SetUnicodeValue(warning.get()); + +Correct: + +.. code-block:: cpp + + constexpr auto warning = u"danger will robinson!"_ns; + ... + // if you'll be using the 'warning' string, you can still use it as before: + foo->SetStringValue(warning); + ... + bar->SetUnicodeValue(warning.get()); + + // alternatively, use the wide string directly: + foo->SetStringValue(u"danger will robinson!"_ns); + ... + + // if a macro is the source of a 8-bit literal and you cannot change it, use + // NS_LITERAL_STRING_FROM_CSTRING, but only if necessary. + #define MY_MACRO_LITERAL "danger will robinson!" + foo->SetStringValue(NS_LITERAL_STRING_FROM_CSTRING(MY_MACRO_LITERAL)); + + // If you need to pass to a raw const char16_t *, there's no benefit to + // go through our string classes at all, just do... + bar->SetUnicodeValue(u"danger will robinson!"); + + // .. or, again, if a macro is the source of a 8-bit literal + bar->SetUnicodeValue(u"" MY_MACRO_LITERAL); + + +Usage of PR_(MAX|MIN|ABS|ROUNDUP) macro calls +--------------------------------------------- + +Use the standard-library functions (``std::max``), instead of +``PR_(MAX|MIN|ABS|ROUNDUP)``. + +Use ``mozilla::Abs`` instead of ``PR_ABS``. All ``PR_ABS`` calls in C++ code have +been replaced with ``mozilla::Abs`` calls, in `bug +847480 <https://bugzilla.mozilla.org/show_bug.cgi?id=847480>`__. All new +code in ``Firefox/core/toolkit`` needs to ``#include "nsAlgorithm.h"`` and +use the ``NS_foo`` variants instead of ``PR_foo``, or +``#include "mozilla/MathAlgorithms.h"`` for ``mozilla::Abs``. + +Use of SpiderMonkey rooting typedefs +------------------------------------ +The rooting typedefs in ``js/public/TypeDecls.h``, such as ``HandleObject`` and +``RootedObject``, are deprecated both in and outside of SpiderMonkey. They will +eventually be removed and should not be used in new code. diff --git a/docs/code-quality/coding-style/coding_style_general.rst b/docs/code-quality/coding-style/coding_style_general.rst new file mode 100644 index 0000000000..950cd6ccd3 --- /dev/null +++ b/docs/code-quality/coding-style/coding_style_general.rst @@ -0,0 +1,18 @@ + +Mode line +~~~~~~~~~ + +Files should have Emacs and vim mode line comments as the first two +lines of the file, which should set ``indent-tabs-mode`` to ``nil``. For new +files, use the following, specifying two-space indentation: + +.. code-block:: cpp + + /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ + /* vim: set ts=2 et sw=2 tw=80: */ + /* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ + +Be sure to use the correct ``Mode`` in the first line, don't use ``C++`` in +JavaScript files. diff --git a/docs/code-quality/coding-style/coding_style_java.rst b/docs/code-quality/coding-style/coding_style_java.rst new file mode 100644 index 0000000000..f2206d8e2d --- /dev/null +++ b/docs/code-quality/coding-style/coding_style_java.rst @@ -0,0 +1,68 @@ +================= +Java Coding style +================= + +- We use the `Java Coding + Style <https://www.oracle.com/technetwork/java/codeconvtoc-136057.html>`__. + Quick summary: + + - FirstLetterUpperCase for class names. + - camelCase for method and variable names. + - One declaration per line: + + .. code-block:: java + + int x, y; // this is BAD! + int a; // split it over + int b; // two lines + +- Braces should be placed like so (generally, opening braces on same + line, closing braces on a new line): + + .. code-block:: java + + public void func(int arg) { + if (arg != 0) { + while (arg > 0) { + arg--; + } + } else { + arg++; + } + } + +- Places we differ from the Java coding style: + + - Start class variable names with 'm' prefix (e.g. + mSomeClassVariable) and static variables with 's' prefix (e.g. + sSomeStaticVariable) + - ``import`` statements: + + - Do not use wildcard imports like \`import java.util.*;\` + - Organize imports by blocks separated by empty line: + org.mozilla.*, android.*, com.*, net.*, org.*, then java.\* + This is basically what Android Studio does by default, except + that we place org.mozilla.\* at the front - please adjust + Settings -> Editor -> Code Style -> Java -> Imports + accordingly. + - Within each import block, alphabetize import names with + uppercase before lowercase. For example, ``com.example.Foo`` is + before ``com.example.bar`` + + - 4-space indents. + - Spaces, not tabs. + - Don't restrict yourself to 80-character lines. Google's Android + style guide suggests 100-character lines, which is also the + default setting in Android Studio. Java code tends to be long + horizontally, so use appropriate judgement when wrapping. Avoid + deep indents on wrapping. Note that aligning the wrapped part of a + line, with some previous part of the line (rather than just using + a fixed indent), may require shifting the code every time the line + changes, resulting in spurious whitespace changes. + +- For additional specifics on Firefox for Android, see the `Coding + Style guide for Firefox on + Android <https://wiki.mozilla.org/Mobile/Fennec/Android#Coding_Style>`__. +- The `Android Coding + Style <https://source.android.com/source/code-style.html>`__ has some + useful guidelines too. diff --git a/docs/code-quality/coding-style/coding_style_js.rst b/docs/code-quality/coding-style/coding_style_js.rst new file mode 100644 index 0000000000..09d7a6fc8a --- /dev/null +++ b/docs/code-quality/coding-style/coding_style_js.rst @@ -0,0 +1,147 @@ +======================= +JavaScript Coding style +======================= + +Coding style +~~~~~~~~~~~~ + +`prettier <https://prettier.io/>`_ is the tool used to reformat the JavaScript code. + + +Methods and functions +~~~~~~~~~~~~~~~~~~~~~ + +In JavaScript, functions should use camelCase, but should not capitalize +the first letter. Methods should not use the named function expression +syntax, because our tools understand method names: + +.. code-block:: cpp + + doSomething: function (aFoo, aBar) { + ... + } + +In-line functions should have spaces around braces, except before commas +or semicolons: + +.. code-block:: cpp + + function valueObject(aValue) { return { value: aValue }; } + + +JavaScript objects +~~~~~~~~~~~~~~~~~~ + +.. code-block:: cpp + + var foo = { prop1: "value1" }; + + var bar = { + prop1: "value1", + prop2: "value2" + }; + +Constructors for objects should be capitalized and use Pascal Case: + +.. code-block:: cpp + + function ObjectConstructor() { + this.foo = "bar"; + } + + +Operators +~~~~~~~~~ + +In JavaScript, overlong expressions not joined by ``&&`` and +``||`` should break so the operator starts on the second line and +starting in the same column as the beginning of the expression in the +first line. This applies to ``?:``, binary arithmetic operators +including ``+``, and member-of operators. Rationale: an operator at the +front of the continuation line makes for faster visual scanning, as +there is no need to read to the end of line. Also there exists a +context-sensitive keyword hazard in JavaScript; see {{bug(442099, "bug", +19)}}, which can be avoided by putting . at the start of a continuation +line, in long member expression. + +In JavaScript, ``==`` is preferred to ``===``. + +Unary keyword operators, such as ``typeof``, should have their operand +parenthesized; e.g. ``typeof("foo") == "string"``. + +Literals +~~~~~~~~ + +Double-quoted strings (e.g. ``"foo"``) are preferred to single-quoted +strings (e.g. ``'foo'``), in JavaScript, except to avoid escaping +embedded double quotes, or when assigning inline event handlers. + + +Prefixes +~~~~~~~~ + +- k=constant (e.g. ``kNC_child``). Not all code uses this style; some + uses ``ALL_CAPS`` for constants. +- g=global (e.g. ``gPrefService``) +- a=argument (e.g. ``aCount``) + +- JavaScript Specific Prefixes + + - \_=member (variable or function) (e.g. ``_length`` or + ``_setType(aType)``) + - k=enumeration value (e.g. ``const kDisplayModeNormal = 0``) + - on=event handler (e.g. ``function onLoad()``) + - Convenience constants for interface names should be prefixed with + ``nsI``: + + .. code-block:: javascript + + const nsISupports = Components.interfaces.nsISupports; + const nsIWBN = Components.interfaces.nsIWebBrowserNavigation; + + + +Other advices +~~~~~~~~~~~~~ + +- Do not compare ``x == true`` or ``x == false``. Use ``(x)`` or + ``(!x)`` instead. ``x == true``, is certainly different from if + ``(x)``! Compare objects to ``null``, numbers to ``0`` or strings to + ``""``, if there is chance for confusion. +- Make sure that your code doesn't generate any strict JavaScript + warnings, such as: + + - Duplicate variable declaration. + - Mixing ``return;`` with ``return value;`` + - Undeclared variables or members. If you are unsure if an array + value exists, compare the index to the array's length. If you are + unsure if an object member exists, use ``"name"`` in ``aObject``, + or if you are expecting a particular type you may use + ``typeof(aObject.name) == "function"`` (or whichever type you are + expecting). + +- Use ``['value1, value2']`` to create a JavaScript array in preference + to using + ``new {{JSxRef("Array", "Array", "Syntax", 1)}}(value1, value2)`` + which can be confusing, as ``new Array(length)`` will actually create + a physically empty array with the given logical length, while + ``[value]`` will always create a 1-element array. You cannot actually + guarantee to be able to preallocate memory for an array. +- Use ``{ member: value, ... }`` to create a JavaScript object; a + useful advantage over ``new {{JSxRef("Object", "Object", "", 1)}}()`` + is the ability to create initial properties and use extended + JavaScript syntax, to define getters and setters. +- If having defined a constructor you need to assign default + properties, it is preferred to assign an object literal to the + prototype property. +- Use regular expressions, but use wisely. For instance, to check that + ``aString`` is not completely whitespace use + ``/\S/.{{JSxRef("RegExp.test", "test(aString)", "", 1)}}``. Only use + {{JSxRef("String.search", "aString.search()")}} if you need to know + the position of the result, or {{JSxRef("String.match", + "aString.match()")}} if you need to collect matching substrings + (delimited by parentheses in the regular expression). Regular + expressions are less useful if the match is unknown in advance, or to + extract substrings in known positions in the string. For instance, + {{JSxRef("String.slice", "aString.slice(-1)")}} returns the last + letter in ``aString``, or the empty string if ``aString`` is empty. diff --git a/docs/code-quality/coding-style/coding_style_python.rst b/docs/code-quality/coding-style/coding_style_python.rst new file mode 100644 index 0000000000..811c02bbe7 --- /dev/null +++ b/docs/code-quality/coding-style/coding_style_python.rst @@ -0,0 +1,71 @@ +=================== +Python Coding style +=================== + +Coding style +~~~~~~~~~~~~ + + :ref:`black` is the tool used to reformat the Python code. + +Linting +~~~~~~~ + +The Python linting is done by :ref:`Flake8` and :ref:`pylint` +They are executed by mozlint both at review phase and in the CI. + +Indentation +~~~~~~~~~~~ + +Four spaces in Python code. + + +Makefile/moz.build practices +---------------------------- + +- Changes to makefile and moz.build variables do not require + build-config peer review. Any other build system changes, such as + adding new scripts or rules, require review from the build-config + team. +- Suffix long ``if``/``endif`` conditionals with #{ & #}, so editors + can display matched tokens enclosing a block of statements. + + :: + + ifdef CHECK_TYPE #{ + ifneq ($(flavor var_type),recursive) #{ + $(warning var should be expandable but detected var_type=$(flavor var_type)) + endif #} + endif #} + +- moz.build are python and follow normal Python style. +- List assignments should be written with one element per line. Align + closing square brace with start of variable assignment. If ordering + is not important, variables should be in alphabetical order. + + .. code-block:: python + + var += [ + 'foo', + 'bar' + ] + +- Use ``CONFIG['TARGET_CPU'] {=arm}`` to test for generic classes of + architecture rather than ``CONFIG['OS_TEST'] {=armv7}`` (re: bug 886689). + + +Other advices +~~~~~~~~~~~~~ + +- Install the + `mozext <https://hg.mozilla.org/hgcustom/version-control-tools/file/default/hgext/mozext>`__ + Mercurial extension, and address every issue reported on commit + or the output of ``hg critic``. +- Follow `PEP 8 <https://www.python.org/dev/peps/pep-0008/>`__. Please run :ref:`black` for this. +- Do not place statements on the same line as ``if/elif/else`` + conditionals to form a one-liner. +- Global vars, please avoid them at all cost. +- Exclude outer parenthesis from conditionals.Use + ``if x > 5:,``\ rather than ``if (x > 5):`` +- Use string formatters, rather than var + str(val). + ``var = 'Type %s value is %d'% ('int', 5).`` +- Testing/Unit tests, please write them and make sure that they are executed in the CI. diff --git a/docs/code-quality/coding-style/css_guidelines.rst b/docs/code-quality/coding-style/css_guidelines.rst new file mode 100644 index 0000000000..bfa2841800 --- /dev/null +++ b/docs/code-quality/coding-style/css_guidelines.rst @@ -0,0 +1,569 @@ +CSS Guidelines +============== + +This document contains guidelines defining how CSS inside the Firefox +codebase should be written, it is notably relevant for Firefox front-end +engineers. + +Basics +------ + +Here are some basic tips that can optimize reviews if you are changing +CSS: + +- Avoid ``!important`` but if you have to use it, make sure it's + obvious why you're using it (ideally with a comment). The + `Overriding CSS`_ section contains more information about this. +- Avoid magic numbers; prefer automatic sizing or alignment methods. + Some examples to avoid: + + - absolutely positioned elements + - hardcoded values such as: ``vertical-align: -2px;`` . The reason + you should avoid such "hardcoded" values is that, they don't + necessarily work for all font-size configurations. + +- Avoid setting styles in JavaScript. It's generally better to set a + class and then specify the styles in CSS. +- ``classList`` is generally better than ``className``. There's less + chance of overwriting an existing class. +- Only use generic selectors such as ``:last-child``, when it is what + you mean semantically. If not, using a semantic class name is more + descriptive and usually better. + +Boilerplate +~~~~~~~~~~~ + +Make sure each file starts with the standard copyright header (see +`License Boilerplate <https://www.mozilla.org/MPL/headers/>`__). + +Before adding more CSS +~~~~~~~~~~~~~~~~~~~~~~ + +It is good practice to check if the CSS that is being written is needed, +it can be the case that a common component has been already written +could be reused with or without changes. Most of the time, the common +component already follows the a11y/theme standards defined in this +guide. So, when possible, always prefer editing common components to +writing your own. + +Also, it is good practice to introduce a common class when the new +element you are styling reuses some styles from another element, this +allows the maintenance cost and the amount of code duplication to be +reduced. + +Formatting +---------- + +Spacing & Indentation +~~~~~~~~~~~~~~~~~~~~~ + +- 2 spaces indentation is preferred +- Add a space after each comma, **except** within color functions: + +.. code:: css + + linear-gradient(to bottom, black 1px, rgba(255,255,255,0.2) 1px) + +- Always add a space before ``!important``. + +Omit units on 0 values +~~~~~~~~~~~~~~~~~~~~~~ + +Do this: + +.. code:: css + + margin: 0; + +Not this: + +.. code:: css + + margin: 0px; + +Use expanded syntax +~~~~~~~~~~~~~~~~~~~ + +It is often harder to understand what the shorthand is doing and the +shorthand can also hide some unwanted default values. It is good to +privilege expanded syntax to make your intentions explicit. + +Do this: + +.. code:: css + + border-color: red; + +Not this: + +.. code:: css + + border: red; + +Put multiple selectors on different lines +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Do this: + +.. code:: css + + h1, + h2, + h3 { + font-family: sans-serif; + text-align: center; + } + +Not this: + +.. code:: css + + h1, h2, h3 { + font-family: sans-serif; + text-align: center; + } + +Naming standards for class names +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +- ``lower-case-with-dashes`` is the most common. +- But ``camelCase`` is also used sometimes. Try to follow the style of + existing or related code. + +Other tips +~~~~~~~~~~ + +- Assume ``="true"`` in attribute selectors. + + - Example: Use ``option[checked]``, not ``option[checked="true"]``. + +- Avoid ID selectors unless it is really the wanted goal, since IDs + have higher specificity and therefore are harder to override. +- Using descendant selectors is good practice for performance when + possible: + + - For example: + ``.autocomplete-item[selected] > .autocomplete-item-title`` would + be more efficient than + ``.autocomplete-item[selected] .autocomplete-item-title`` + +Overriding CSS +-------------- + +Before overriding any CSS rules, check whether overriding is really +needed. Sometimes, when copy-pasting older code, it happens that the +code in question contains unnecessary overrides. This could be because +the CSS that it was overriding got removed in the meantime. In this +case, dropping the override should work. + +It is also good practice to look at whether the rule you are overriding +is still needed: maybe the UX spec for the component has changed and +that rule can actually be updated or removed. When this is the case, +don't be afraid to remove or update that rule. + +Once the two things above have been checked, check if the other rule you +are overriding contains ``!important``, if that is case, try putting it +in question, because it might have become obsolete. + +Afterwards, check the specificity of the other selector; if it is +causing your rule to be overridden, you can try reducing its +specificity, either by simplifying the selector or by changing where the +rule is placed in the stylesheet. If this isn't possible, you can also +try introducing a ``:not()`` to prevent the other rule from applying, +this is especially relevant for different element states (``:hover``, +``:active``, ``[checked]`` or ``[disabled]``). However, never try to +increase the selector of the rule you are adding as it can easily become +hard to understand. + +Finally, once you have checked all the things above, you can permit +yourself to use ``!important`` along with a comment why it is needed. + +Using CSS variables +------------------- + +Adding new variables +~~~~~~~~~~~~~~~~~~~~ + +Before adding new CSS variables, please consider the following +questions: + +#. **Is the variable value changed at runtime?** + *(Either from JavaScript or overridden by another CSS file)* + **If the answer is no**, consider using a preprocessor variable or + inlining the value. + +#. **Is the variable value used multiple times?** + **If the answer is no and the value isn't changed at runtime**, then + you likely don't need a CSS variable. + +#. **Is there an alternative to using the variable like inheriting or + using the ``currentcolor`` keyword?** + Using inheriting or using ``currentcolor`` will prevent repetition of + the value and it is usually good practice to do so. + +In general, it's good to first think of how some CSS could be written +cleanly without the CSS variable(s) and then think of how the CSS +variable could improve that CSS. + +Using variables +~~~~~~~~~~~~~~~ + +Use the variable according to its naming +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +Do this: + +.. code:: css + + xul|tab:hover { + background-color: var(--in-content-box-background-hover); + } + +Not this: + +.. code:: css + + #certificateErrorDebugInformation { + background-color: var(--in-content-box-background-hover); + } + +Localization +------------ + +Text Direction +~~~~~~~~~~~~~~ + +- For margins, padding and borders, use + ``inline-start``/``inline-end`` rather than ``left``/``right``. + *Example:* Use ``margin-inline-start: 3px;`` instead of + ``margin-left: 3px``. +- For RTL-aware positioning (left/right), use + ``inset-inline-start``/``inset-inline-end``. +- For RTL-aware float layouts, ``float: inline-start|inline-end`` can + be used instead of ``float: left|right``. +- The RTL-aware equivalents of + ``border-{top/bottom}-{left/right}-radius`` are + ``border-{start/end}-{start/end}-radius`` +- When there is no special RTL-aware property available, use the pseudo + ``:-moz-locale-dir(ltr|rtl)`` (for XUL files) or ``:dir(ltr|rtl)`` + (for HTML files). +- Remember that while a tab content's scrollbar still shows on the + right in RTL, an overflow scrollbar will show on the left. +- Write ``padding: 0 3px 4px;`` instead of + ``padding: 0 3px 4px 3px;``. This makes it more obvious that the + padding is symmetrical (so RTL won't be an issue). + +.. note:: + + See `CSS Logical Properties and + Values <https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Logical_Properties>`__ + for more information. + +Writing cross-platform CSS +-------------------------- + +Firefox supports many different platforms and each of those platforms +can contain many different configurations: + +- Windows 7, 8 and 10 + + - Default theme + - Aero basic (Windows 7, 8) + - Windows classic (Windows 7) + - High contrast (All versions) + +- Linux +- macOS + +File structure +~~~~~~~~~~~~~~ + +- The ``browser/`` directory contains styles specific to Firefox +- The ``toolkit/`` directory contains styles that are shared across all + toolkit applications (Thunderbird and SeaMonkey) + +Under each of those two directories, there is a ``themes`` directory +containing 4 sub-directories: + +- ``shared`` +- ``linux`` +- ``osx`` +- ``windows`` + +The ``shared`` directories contain styles shared across all 3 platforms, +while the other 3 directories contain styles respective to their +platform. + +For new CSS, when possible try to privilege using the ``shared`` +directory, instead of writing the same CSS for the 3 platform specific +directories, especially for large blocks of CSS. + +Content CSS vs. Theme CSS +^^^^^^^^^^^^^^^^^^^^^^^^^ + +The following directories also contain CSS: + +- ``browser/base/content/`` +- ``toolkit/content/`` + +These directories contain content CSS, that applies on all platforms, +which is styling deemed to be essential for the browser to behave +correctly. To determine whether some CSS is theme-side or content-side, +it is useful to know that certain CSS properties are going to lean one +way or the other: color - 99% of the time it will be theme CSS, overflow +- 99% content. + ++-----------------+--------------+----------------+----------------+ +| 99% theme | 70% theme | 70% content | 99% content | ++=================+==============+================+================+ +| font-\*, color, | line-height, | cursor, width, | overflow, | +| \*-color, | padding, | max-width, | direction, | +| border-\*, | margin | top, | display, | +| -moz-appearance | | bottom [2]_, | \*-align, | +| [1]_ | | etc | align-\*, | +| | | | \*-box-\*, | +| | | | flex-\*, order | ++-----------------+--------------+----------------+----------------+ + +If some CSS is layout or functionality related, then it is likely +content CSS. If it is esthetics related, then it is likely theme CSS. + +When importing your stylesheets, it's best to import the content CSS +before the theme CSS, that way the theme values get to override the +content values (which is probably what you want), and you're going to +want them both after the global values, so your imports will look like +this: + +.. code:: html + + <?xml-stylesheet href="chrome://global/skin/global.css" type="text/css"?> + <?xml-stylesheet href="chrome://browser/content/path/module.css" type="text/css"?> + <?xml-stylesheet href="chrome://browser/skin/path/module.css" type="text/css"?> + +.. [1] -moz-appearance is tricky. Generally, when specifying + -moz-appearance: foo; you're giving hints as to how something should + act, however -moz-appearance: none; is probably saying 'ignore + browser preconceptions - I want a blank sheet', so that's more + visual. However -moz-appearance values aren't implemented and don't + behave consistently across platforms, so idealism aside + -moz-appearance should always be in theme CSS. + +.. [2] However there is probably a better way than using absolute + positioning. + +Colors +~~~~~~ + +For common areas of the Firefox interface (panels, toolbar buttons, +etc.), mozilla-central often comes with some useful CSS variables that +are adjusted with the correct values for different platform +configurations, so using those CSS variables can definitively save some +testing time, as you can assume they already work correctly. + +Using the ``currentcolor`` keyword or inheriting is also good practice, +because sometimes the needed value is already in the color or on the +parent element. This is especially useful in conjunction with icons +using ``-moz-context-properties: fill;`` where the icon can adjust to +the right platform color automatically from the text color. It is also +possible to use ``currentcolor`` with other properties like +``opacity`` or ``fill-opacity`` to have different +opacities of the platform color. + +High contrast mode +~~~~~~~~~~~~~~~~~~ + +Content area +^^^^^^^^^^^^ + +On Windows high contrast mode, in the content area, Gecko does some +automatic color adjustments regarding page colors. Part of those +adjustments include making all ``box-shadow`` invisible, so this is +something to be aware of if you create a focus ring or a border using +the ``box-shadow`` property: consider using a ``border`` or an +``outline`` if you want the border/focus ring to stay visible in +high-contrast mode. An example of such bug is `bug +1516767 <https://bugzilla.mozilla.org/show_bug.cgi?id=1516767>`__. + +Another adjustment to be aware of is that Gecko removes all the +``background-image`` when high contrast mode is enabled. Consider using +an actual ``<img>`` tag (for HTML documents) or ``list-style-image`` +(for XUL documents) if rendering the image is important. + +If you are not using Windows, one way to test against those adjustments +on other platforms is: + +- Going to about:preferences +- Clicking on the "Colors..." button in the "Fonts & Colors" + sub-section of the "Language and Appearance" section +- Under "Override the colors specified by the page with your selections + above", select the "Always" option + +Chrome area +^^^^^^^^^^^ + +The automatic adjustments previously mentioned only apply to pages +rendered in the content area. The chrome area of Firefox uses colors as +authored, which is why using pre-defined variables, ``currentcolor`` or +inheritance is useful to integrate with the system theme with little +hassle. + +If not, as a last resort, using `system +colors <https://developer.mozilla.org/en-US/docs/Web/CSS/color_value#system_colors>`__ +also works for non-default Windows themes or Linux. In general, the +following colors are used: + +- ``-moz-Field``: textbox or field background colors, also used as the + background color of listboxes or trees. +- ``-moz-FieldText``: textbox or field text colors, also used as the + text color of listboxes or trees. +- ``-moz-Dialog``: window or dialog background color. +- ``-moz-DialogText``: window or dialog text color. +- ``GrayText``: used on disabled items as text color. Do not use it on + text that is not disabled to desemphsize text, because it does not + guarantee a sufficient contrast ratio for non-disabled text. +- ``ThreeDShadow``: Used as border on elements. +- ``ThreeDLightShadow``: Used as light border on elements. + +Using the background/text pairs is especially important to ensure the +contrast is respected in all situations. Never mix custom text colors +with a system background color and vice-versa. + +Note that using system colors is only useful for the chrome area, since +content area colors are overridden by Gecko anyway. + +Writing media queries +~~~~~~~~~~~~~~~~~~~~~ + +Boolean media queries +^^^^^^^^^^^^^^^^^^^^^ + +Do this: + +.. code:: css + + @media (-moz-mac-yosemite-theme: 0) { + +Not this: + +.. code:: css + + @media not all and (-moz-mac-yosemite-theme) { + +Privilege CSS for most common configuration +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +It is better to put the most common configuration (latest version of an +OS, or default theme for example) outside of the media query. In the +following example, ``-moz-mac-yosemite-theme`` targets macOS 10.10 and +higher, so it should be privileged over the styling for macOS 10.9. + +Do this: + +.. code:: css + + @media (-moz-mac-yosemite-theme: 0) { + #placesList { + box-shadow: inset -2px 0 0 hsla(0,0%,100%,.2); + } + } + +Not this: + +.. code:: css + + #placesList { + box-shadow: inset -2px 0 0 hsla(0,0%,100%,.2); + } + + @media (-moz-mac-yosemite-theme) { + #placesList { + box-shadow: none; + } + } + +Theme support +------------- + +Firefox comes built-in with 3 themes: default, light and dark. The +built-in light/dark themes are a bit special as they load the +``compacttheme.css`` stylesheet. In addition to this, Firefox supports a +variety of WebExtension themes that can be installed from AMO. For +testing purposes, `here is an example of a WebExtension +theme. <https://addons.mozilla.org/en-US/firefox/addon/arc-dark-theme-we/>`__ + +Writing theme-friendly CSS +~~~~~~~~~~~~~~~~~~~~~~~~~~ + +- Some CSS variables that are pre-adjusted for different platforms are + also pre-adjusted for themes, so it's again a good idea to use them + for theme support. +- The text color of elements often contains valuable information from + the theme colors, so ``currentcolor``/inheritance is again a good + idea for theme support. +- Never write CSS specially for the built-in light/dark theme in + ``compacttheme.css`` unless that CSS isn't supposed to affect + WebExtension themes. +- These selectors can be used to target themed areas, though in general it's + recommended to try to avoid them and use ``light-dark()`` to get the right + colors automatically: + + - ``:root[lwt-toolbar-field="light/dark"]``: explicitly light or dark address bar and + searchbar. + - ``:root[lwt-toolbar-field-focus="light/dark"]``: explicitly light or dark address bar and + searchbar in the focused state. + - ``:root[lwt-popup="light/dark"]``: explicitly light or dark arrow panels + and autocomplete panels. + - ``:root[lwt-sidebar="light/dark"]``: explicitly light or dark sidebars. + +- If you'd like a different shade of a themed area and no CSS variable + is adequate, using colors with alpha transparency is usually a good + idea, as it will preserve the original theme author's color hue. + +Variables +~~~~~~~~~ + +For clarity, CSS variables that are only used when a theme is enabled +have the ``--lwt-`` prefix. + +Layout & performance +-------------------- + +Layout +~~~~~~ + +Mixing XUL flexbox and HTML flexbox can lead to undefined behavior. + +CSS selectors +~~~~~~~~~~~~~ + +When targeting the root element of a page, using ``:root`` is the most +performant way of doing so. + +Reflows and style flushes +~~~~~~~~~~~~~~~~~~~~~~~~~ + +See :ref:`Performance best practices for Firefox front-end engineers` +for more information about this. + +Misc +---- + +Text aliasing +~~~~~~~~~~~~~ + +When convenient, avoid setting the ``opacity`` property on +text as it will cause text to be aliased differently. + +HDPI support +~~~~~~~~~~~~ + +It's recommended to use SVG since it keeps the CSS clean when supporting +multiple resolutions. See the :ref:`SVG Guidelines` for more information +on SVG usage. + +However, if only 1x and 2x PNG assets are available, you can use this +``@media`` query to target higher density displays (HDPI): + +.. code:: css + + @media (min-resolution: 1.1dppx) diff --git a/docs/code-quality/coding-style/format_cpp_code_with_clang-format.rst b/docs/code-quality/coding-style/format_cpp_code_with_clang-format.rst new file mode 100644 index 0000000000..6b87b0c11e --- /dev/null +++ b/docs/code-quality/coding-style/format_cpp_code_with_clang-format.rst @@ -0,0 +1,272 @@ +===================================== +Formatting C++ Code With clang-format +===================================== + +Mozilla uses the Google coding style for whitespace, which is enforced +using `clang-format <https://clang.llvm.org/docs/ClangFormat.html>`__. A +specific version of the binary will be installed when +``./mach clang-format`` or ``./mach bootstrap`` are run. We build our +own binaries and update them as needed. + +Options are explicitly defined `in clang-format +itself <https://github.com/llvm-mirror/clang/blob/e8a55f98df6bda77ee2eaa7f7247bd655f79ae0e/lib/Format/Format.cpp#L856>`__. +If the options are changed in clang upstream, this might cause some +changes in the Firefox tree. For this reason, it is best to use the +mozilla-provided binaries. + +Manual formatting +----------------- + +We provide a mach subcommand for running clang-format from the +command-line. This wrapper handles ensuring the correct version of +clang-format is installed and run. + +If clang-format isn’t installed, the binaries will be automatically +downloaded from taskcluster and installed into ~/.mozbuild. We build our +own clang-format binaries. + + +Formatting local changes +~~~~~~~~~~~~~~~~~~~~~~~~ + +:: + + $ ./mach clang-format + +When run without arguments, it will run on a local diff. This could miss +some reformatting (for example, when blocks are touched). +(`searchfox <https://searchfox.org/mozilla-central/rev/501eb4718d73870892d28f31a99b46f4783efaa0/python/mozbuild/mozbuild/code-analysis/mach_commands.py#1620>`__) + + +Formatting specific paths +~~~~~~~~~~~~~~~~~~~~~~~~~ + +:: + + $ ./mach clang-format -p <path> # Format <path> in-place + $ ./mach clang-format -p <path> -s # Show changes + +The command also accepts a ``-p`` argument to reformat a specific +directory or file, and a ``-s`` flag to show the changes instead of +applying them to the working directory +(`searchfox <https://searchfox.org/mozilla-central/rev/501eb4718d73870892d28f31a99b46f4783efaa0/python/mozbuild/mozbuild/code-analysis/mach_commands.py#1633>`__) + + +Formatting specific commits / revisions +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +:: + + $ ./mach clang-format -c HEAD # Format a single git commit + $ ./mach clang-format -c HEAD~~..HEAD # Format a range of git commits + $ ./mach clang-format -c . # Format a single mercurial revision + +The command accepts a ``-c`` argument that takes a revision number or +commit range, and will format the lines modified by those commits. +(`searchfox <https://searchfox.org/mozilla-central/rev/501eb4718d73870892d28f31a99b46f4783efaa0/python/mozbuild/mozbuild/code-analysis/mach_commands.py#1635>`__) + + +Scripting Clang-Format +~~~~~~~~~~~~~~~~~~~~~~ + +Clang format expects that the path being passed to it is the path +on-disk. If this is not the case, for example when formatting a +temporary file, the "real" path must be specified. This can be done with +the ``--assume-filename <path>`` argument. + + +Configuring the clang-format commit hook +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +To run clang-format at commit phase, run ``mach bootstrap`` or just add +the following line in the ``hgrc`` file: + +.. code:: ini + + [extensions] + clang-format = ~/.mozbuild/version-control-tools/hgext/clang-format + +We use a hg extension as they are more flexible than hooks. + +With git, the configuration is the following: + +:: + + # From the root git directory: + $ ln -s $(pwd)/tools/lint/hooks_clang_format.py .git/hooks/pre-commit + +You'll likely need to install the ``python-hglib`` package for your OS, +or else you may get errors like ``abort: No module named hglib.client!`` +when you try to commit. + + +Editor integration +------------------ + +It is possible to configure many editors to support running +``clang-format`` automatically on save, or when run from within the +editor. + + +Editor plugins +~~~~~~~~~~~~~~ + +- `Atom <https://atom.io/packages/clang-format>`__ +- `BBEdit <http://clang.llvm.org/docs/ClangFormat.html#bbedit-integration>`__ + + - `clang-format-bbedit.applescript <https://raw.githubusercontent.com/llvm-mirror/clang/master/tools/clang-format/clang-format-bbedit.applescript>`__ + +- Eclipse + + - Install the + `CppStyle <https://marketplace.eclipse.org/content/cppstyle>`__ + plugin + - In Preferences -> C/C++ -> CppStyle, set the clang-format path to + ~/.mozbuild/clang-tools/clang-tidy/bin/clang-format + - (Optional) check "Run clang-format on file save" + +- `Emacs <http://clang.llvm.org/docs/ClangFormat.html#emacs-integration>`__ + + - `clang-format.el <https://raw.githubusercontent.com/llvm-mirror/clang/master/tools/clang-format/clang-format.el>`__ + (Or install + `clang-format <http://melpa.org/#/clang-format>`__ from MELPA) + - `google-c-style <http://melpa.org/#/google-c-style>`__ from MELPA + +- `Sublime Text <https://packagecontrol.io/packages/Clang%20Format>`__ + + - `alternative + tool <https://github.com/rosshemsley/SublimeClangFormat>`__ + +- `Vim <http://clang.llvm.org/docs/ClangFormat.html#vim-integration>`__ + + - `clang-format.py <https://raw.githubusercontent.com/llvm-mirror/clang/master/tools/clang-format/clang-format.py>`__ + - `vim-clang-format <https://github.com/rhysd/vim-clang-format>`__ + +- `Visual + Studio <https://marketplace.visualstudio.com/items?itemName=LLVMExtensions.ClangFormat>`__ + + - `llvm.org plugin <http://llvm.org/builds/>`__ + - `Integrated support in Visual Studio + 2017 <https://blogs.msdn.microsoft.com/vcblog/2018/03/13/clangformat-support-in-visual-studio-2017-15-7-preview-1/>`__ + +- `Visual Studio + Code <https://marketplace.visualstudio.com/items?itemName=xaver.clang-format>`__ +- `XCode <https://github.com/travisjeffery/ClangFormat-Xcode>`__ +- `Script for patch + reformatting <http://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting>`__ + + - `clang-format-diff.py <https://raw.githubusercontent.com/llvm-mirror/clang/master/tools/clang-format/clang-format-diff.py>`__ + + +Configuration +~~~~~~~~~~~~~ + +These tools generally run clang-format themselves, and won't use +``./mach clang-format``. The binary installed by our tooling will be +located at ``~/.mozbuild/clang-tools/clang-tidy/bin/clang-format``. + +You typically shouldn't need to specify any other special configuration +in your editor besides the clang-format binary. Most of the +configuration that clang-format relies on for formatting is stored +inside our source tree. More specifically, using the .clang-format file +located in the root of the repository. Please note that this doesn't +include the list of ignored files and directories (provided by +.clang-format-ignore which is a feature provided by the mach command +wrapper). + +Coding style configuration is done within clang-format itself. When we +change the configuration (incorrect configuration, new feature in clang, +etc), we use `local +overrides <https://searchfox.org/mozilla-central/rev/501eb4718d73870892d28f31a99b46f4783efaa0/.clang-format>`__. + + +Ignored files & directories +~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +We maintain a `list of ignored directories and +files <https://searchfox.org/mozilla-central/rev/501eb4718d73870892d28f31a99b46f4783efaa0/.clang-format-ignore>`__, +which is used by ``./mach clang-format``. This is generally only used +for code broken by clang-format, and third-party code. + + +Ignored code hunks +~~~~~~~~~~~~~~~~~~ + +Sections of code may have formatting disabled using comments. If a +section must not be formatted, the following comments will disable the +reformat: + +:: + + // clang-format off + my code which should not be reformatted + // clang-format on + +You can find an `example of code not +formatted <https://searchfox.org/mozilla-central/rev/501eb4718d73870892d28f31a99b46f4783efaa0/xpcom/io/nsEscape.cpp#22>`__. + + +Merging formatted and unformatted code +-------------------------------------- + +During the transition to using chromium style enforced by clang-format +for all code in tree, it will often be necessary to rebase non-formatted +code onto a formatted tree. + + +Mercurial +~~~~~~~~~ + +The ``format-source`` extension, now bundled with +``version-control-tools``, and installed by ``./mach bootstrap``, may be +used to seamlessly handle this situation. More details may be found in +this +`document <https://docs.google.com/document/d/13AwAsvKMhH0mflDlfatBqn6LmZHiQih76oxM4zfrPl4/edit>`__. + +The parent changeset of the reformat has been tagged as +``PRE_TREEWIDE_CLANG_FORMAT``. + + +Git +~~~ + +To perform a rebase onto mozilla-central after the merge, a handy merge +driver, ``clang-format-merge``, has been written: + +.. code:: shell + + $ git clone https://github.com/emilio/clang-format-merge + $ /path/to/clang-format-merge/git-wrapper rebase <upstream> + +The wrapper should clean up after itself, and the clone may be deleted +after the rebase is complete. + + +Ignore lists +------------ + +To make sure that the blame/annotate features of Mercurial or git aren't +affected. Two files are maintained to keep track of the reformatting +commits. + + +With Mercurial +~~~~~~~~~~~~~~ + +| The list is stored in + `https://searchfox.org/mozilla-central/source/.hg-annotate-ignore-revs </en-US/docs/>`__ +| Commit messages should also contain the string ``# ignore-this-changeset`` + +The syntax in this file is generated using the following syntax: + +:: + + $ hg log --template '{node} - {author|person} - {desc|strip|firstline}\n' + +With git +~~~~~~~~ + +The list is stored in +`https://searchfox.org/mozilla-central/source/.git-blame-ignore-revs </en-US/docs/>`__ +and contains git revisions for both gecko-dev and the git cinnabar +repository. diff --git a/docs/code-quality/coding-style/index.rst b/docs/code-quality/coding-style/index.rst new file mode 100644 index 0000000000..e62ce910ca --- /dev/null +++ b/docs/code-quality/coding-style/index.rst @@ -0,0 +1,20 @@ +Coding style +============ + +Firefox code is using different programming languages. +For each language, we are enforcing a specific coding style. + +Getting Help +------------ + +If you need help or have questions, please don’t hesitate to contact us via Matrix +in the "Lint and Formatting" room +(`#lint:mozilla.org <https://chat.mozilla.org/#/room/#lint:mozilla.org>`_). + + +.. toctree:: + :caption: Coding Style User Guide + :maxdepth: 2 + :glob: + + * diff --git a/docs/code-quality/coding-style/rtl_guidelines.rst b/docs/code-quality/coding-style/rtl_guidelines.rst new file mode 100644 index 0000000000..8b2a6bcf04 --- /dev/null +++ b/docs/code-quality/coding-style/rtl_guidelines.rst @@ -0,0 +1,356 @@ +RTL Guidelines +============== + +RTL languages such as Arabic, Hebrew, Persian and Urdu are read and +written from right-to-left, and the user interface for these languages +should be mirrored to ensure the content is easy to understand. + +When a UI is changed from LTR to RTL (or vice-versa), it’s often called +mirroring. An RTL layout is the mirror image of an LTR layout, and it +affects layout, text, and graphics. + +In RTL, anything that relates to time should be depicted as moving from +right to left. For example, forward points to the left, and backwards +points to the right. + +Mirroring layout +~~~~~~~~~~~~~~~~ + +When a UI is mirrored, these changes occur: + +- Text fields icons are displayed on the opposite side of a field +- Navigation buttons are displayed in reverse order +- Icons that communicate direction, like arrows, are mirrored +- Text is usually aligned to the right + +In CSS, while it's possible to apply a rule for LTR and a separate one +specifically for RTL, it's usually better to use CSS `Logical Properties <https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Logical_Properties>`_ +which provide the ability to control layout through logical, rather than +physical mappings. + ++---------------------------------------------------------+--------------------------------------------------+ +| Do | Don't do | ++---------------------------------------------------------+--------------------------------------------------+ +| ``margin-inline-start: 5px`` | ``margin-left: 5px`` | ++---------------------------------------------------------+--------------------------------------------------+ +| ``padding-inline-end: 5px`` | ``padding-right: 5px`` | ++---------------------------------------------------------+--------------------------------------------------+ +| ``float: inline-start`` | ``float: left`` | ++---------------------------------------------------------+--------------------------------------------------+ +| ``inset-inline-start: 5px`` | ``left: 5px`` | ++---------------------------------------------------------+--------------------------------------------------+ +| ``border-inline-end: 1px`` | ``border-right: 1px`` | ++---------------------------------------------------------+--------------------------------------------------+ +| ``border-{start/end}-{start/end}-radius: 2px`` | ``border-{top/bottom}-{left/right}-radius: 2px`` | ++---------------------------------------------------------+--------------------------------------------------+ +| ``padding: 1px 2px`` | ``padding: 1px 2px 1px 2px`` | ++---------------------------------------------------------+--------------------------------------------------+ +| ``margin-block: 1px 3px`` && ``margin-inline: 4px 2px`` | ``margin: 1px 2px 3px 4px`` | ++---------------------------------------------------------+--------------------------------------------------+ +| ``text-align: start`` or ``text-align: match-parent`` | ``text-align: left`` | +| (depends on the context) | | ++---------------------------------------------------------+--------------------------------------------------+ + +When there is no special RTL-aware property available, or when +left/right properties must be used specifically for RTL, use the pseudo +``:-moz-locale-dir(rtl)`` (for XUL documents) or ``:dir(rtl)`` (for HTML +documents). + +For example, this rule covers LTR to display searchicon.svg 7 pixels +from the left: + +.. code:: css + + .search-box { + background-image: url(chrome://path/to/searchicon.svg); + background-position: 7px center; + } + +but an additional rule is necessary to cover RTL and place the search +icon on the right: + +.. code:: css + + .search-box:dir(rtl) { + background-position-x: right 7px; + } + +.. warning:: + + It may be inappropriate to use logical properties when embedding LTR + within RTL contexts. This is described further in the document. + +Mirroring elements +~~~~~~~~~~~~~~~~~~ + +RTL content also affects the direction in which some icons and images +are displayed, particularly those depicting a sequence of events. + +What to mirror +^^^^^^^^^^^^^^ + +- Icons or animations that imply directionality or motion like + back/forward buttons or progress bars +- Icons that imply text direction, like + `reader-mode.svg <https://searchfox.org/mozilla-central/rev/f9beb753a84aa297713d1565dcd0c5e3c66e4174/browser/themes/shared/icons/reader-mode.svg>`__ +- Icons that imply location of UI elements in the screen, like + `sidebars-right.svg <https://searchfox.org/mozilla-central/rev/74cc0f4dce444fe0757e2a6b8307d19e4d0e0212/browser/themes/shared/icons/sidebars-right.svg>`__, + `open-in-new.svg <https://searchfox.org/mozilla-central/rev/f9beb753a84aa297713d1565dcd0c5e3c66e4174/toolkit/themes/shared/icons/open-in-new.svg>`__, + `default theme's preview.svg <https://searchfox.org/mozilla-central/rev/f9beb753a84aa297713d1565dcd0c5e3c66e4174/toolkit/mozapps/extensions/default-theme/preview.svg>`__ + or + `pane-collapse.svg <https://searchfox.org/mozilla-central/rev/74cc0f4dce444fe0757e2a6b8307d19e4d0e0212/devtools/client/debugger/images/pane-collapse.svg>`__ +- Icons representing objects that are meant to be handheld should look + like they're being right-handed, like the `magnifying glass + icon <https://searchfox.org/mozilla-central/rev/e7c61f4a68b974d5fecd216dc7407b631a24eb8f/toolkit/themes/windows/global/icons/search-textbox.svg>`__ +- Twisties in their collapsed state. Note that if the context in which + they appear is LTR (e.g. code in a devtools HTML view), they should + not be mirrored, even if the user might be using an RTL locale. + +What NOT to mirror +^^^^^^^^^^^^^^^^^^ + +- Text/numbers +- Icons containing text/numbers +- Icons/animations that are direction neutral +- Icons that wouldn't look differently if they'd be mirrored, like `X + buttons <https://searchfox.org/mozilla-central/rev/a78233c11a6baf2c308fbed17eb16c6e57b6a2ac/devtools/client/debugger/images/close.svg>`__ + or the `bookmark + star <https://searchfox.org/mozilla-central/rev/a78233c11a6baf2c308fbed17eb16c6e57b6a2ac/browser/themes/shared/icons/bookmark-hollow.svg>`__ + icon, or any other symmetric icon +- Icons that should look the same as LTR, like icons related to code + (which is always LTR) like + `tool-webconsole.svg <https://searchfox.org/mozilla-central/rev/74cc0f4dce444fe0757e2a6b8307d19e4d0e0212/devtools/client/themes/images/tool-webconsole.svg>`__ +- Checkmark icons +- Video/audio player controls +- Product logos +- Order of size dimensions (e.g., ``1920x1080`` should not become + ``1080x1920``) +- Order of size units (e.g., ``10 px`` should not become ``px 10`` + (unless the size unit is localizable)) + +How +^^^ + +The most common way to mirror images is by flipping the X axis: + +.. code:: css + + transform: scaleX(-1); + +Or, if you're already using ``transform`` with a different value on the same +element, you can also use `scale`: + +.. code:: css + + scale: -1 1; + +Note that mirroring images that way doesn't work when the image is a part of +an element with text using ``background-image``, because then the text would +be mirrored along with the image, and the image would be positioned incorrectly. +For such cases, try to use a different method for displaying the image, +like having it as an element all on its own. +If that's not possible, add a separate pre-mirrored image asset and specify +it in a separate ``:dir(rtl)`` rule: + +.. code:: css + + .element-with-icon { + background-image: url("path/to/image/image.svg"); + } + + .element-with-icon:dir(rtl) { + background-image: url("path/to/image/image-rtl.svg"); + } + +For animations like a progress bar, when using ``@keyframes`` to change +the ``transform: translateX()`` states, make sure to add a different +``@keyframes`` suited for RTL, and target that in a separate ``:dir()`` rule: + +.. code:: css + + #progressbar { + animation: progressbar-animation 1s linear infinite; + } + + #progressbar:dir(rtl) { + animation-name: progressbar-animation-rtl; + } + + @keyframes progressbar-animation { + 0% { + transform: translateX(-100px); + } + 100% { + transform: translateX(0); + } + } + + @keyframes progressbar-animation-rtl { + 0% { + transform: translateX(100px); + } + 100% { + transform: translateX(0); + } + } + +Likewise, if you're using ``transform-origin``, make sure to specify the +correct origin for RTL: + +.. code:: css + + #progressbar { + transform-origin: 0 0; + } + + #progressbar:dir(rtl) { + transform-origin: 100% 0; + } + +LTR text inside RTL contexts +~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +By default, in RTL locales, some symbols like ``/`` and ``.`` will be moved +around and won't be displayed in the order that they were typed in. This +may be problematic for URLs for instance, where you don't want dots to +change position. + +Here's a non-exhaustive list of elements that should be displayed like +they would be in LTR locales: + +- Paths (e.g., C:\\Users\\username\\Desktop) +- Full URLs +- Code and code containers (like the DevTools' Inspector or the CSS + rules panel) +- about:config preference names and values +- Telephone numbers +- Usernames & passwords (most sites on the web expect LTR + usernames/passwords, but there may be exceptions) +- Other text fields where only LTR text is expected + +To make sure these are displayed correctly, you can use one of the +following on the relevant element: + +- ``direction: ltr`` +- ``dir="ltr"`` in HTML + +Since the direction of such elements is forced to LTR, the text will +also be aligned to the left, which is undesirable from an UI +perspective, given that is inconsistent with the rest of the RTL UI +which has text usually aligned to the right. You can fix this using +``text-align: match-parent``. In the following screenshot, both text +fields (username and password) and the URL have their direction set to +LTR (to display text correctly), but the text itself is aligned to the +right for consistency with the rest of the UI: + +.. image:: about-logins-rtl.png + :alt: about:logins textboxes in RTL layout + +However, since the direction in LTR, this also means that the start/end +properties will correspond to left/right respectively, which is probably +not what you expect. This means you have to use extra rules instead of +using logical properties. + +Here's a full code example: + +.. code:: css + + .url { + direction: ltr; /* Force text direction to be LTR */ + + /* `start` (the default value) will correspond to `left`, + * so we match the parent's direction in order to align the text to the right */ + text-align: match-parent; + } + + /* :dir(ltr/rtl) isn't meaningful on .url, since it has direction: ltr, hence + * why it is matched on .container. */ + .container:dir(ltr) .url { + padding-left: 1em; + } + + .container:dir(rtl) .url { + padding-right: 1em; + } + +.. note:: + + The LTR rule is separate from the global rule to avoid having the + left padding apply on RTL without having to reset it in the RTL rule. + +Auto-directionality +^^^^^^^^^^^^^^^^^^^ + +Sometimes, the text direction on an element should vary dynamically +depending on the situation. This can be the case for a search input for +instance, a user may input a query in an LTR language, but may also +input a query in a RTL language. In this case, the search input has to +dynamically pick the correct directionality based on the first word, in +order to display the query text correctly. The typical way to do this is +to use ``dir="auto"`` in HTML. It is essential that +``text-align: match-parent`` is set, to avoid having the text alignment +change based on the query, and logical properties also cannot be used on +the element itself given they can change meaning depending on the query. + +Testing +~~~~~~~ + +To test for RTL layouts in Firefox, you can go to about:config and +set ``intl.l10n.pseudo`` to ``bidi``, or select the ``Enable "bidi" locale`` +option in the 3-dots menu in the :doc:`Browser Toolbox </devtools-user/browser_toolbox/index>`. +The Firefox UI should immediately flip, but a restart may be required +to take effect in some Firefox features and interactions. + +.. note:: + + When testing with ``intl.l10n.pseudo`` set to ``bidi``, you may see some + oddities regarding text ordering due to the nature of displaying LTR + text in RTL layout. + + .. image:: about-protections-rtl.png + :alt: about:protections in RTL layout- English vs. Hebrew + + This shouldn't be an issue when using an actual RTL build or language pack. + +How to spot RTL-related issues +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +- Punctuation marks should appear on the left side of a + word/sentence/paragraph on RTL, so if a *localizable* string appears + in the UI with a dot, colon, ellipsis, question or exclamation mark + on the right side of the text, this probably means that the text + field is forced to be displayed as LTR. +- If icons/images/checkmarks do not appear on the opposite side of + text, when compared to LTR. +- If buttons (like the close button, "OK" and "Cancel" etc.) do not + appear on the opposite side of the UI and not in the opposite order, + when compared to LTR. +- If paddings/margins/borders are not the same from the opposite side, + when compared to LTR. +- Although Hebrew uses ``1 2 3``, all the other RTL locales we support + should use ``١ ٢ ٣`` as digits. So if you see ``1 2 3`` on any such + locale, that likely indicates a bug. +- If navigating in the UI using the left/right arrow keys does not + select the correct element (i.e., pressing Left selects an item on + the right). +- If navigating in the UI using the Tab key does not focus elements + from right to left, in an RTL context. +- If code is displayed as RTL (e.g., ``;padding: 20px`` - the semicolon + should appear on the right side of the code). Code can still be + aligned to the right if it appears in an RTL context. + +See also +~~~~~~~~ + +- `RTL Best + Practices <https://docs.google.com/document/d/1Rc8rvwsLI06xArFQouTinSh3wNte9Sqn9KWi1r7xY4Y/edit#heading=h.pw54h41h12ct>`__ +- Building RTL-Aware Web Apps & Websites: `Part + 1 <https://hacks.mozilla.org/2015/09/building-rtl-aware-web-apps-and-websites-part-1/>`__, + `Part + 2 <https://hacks.mozilla.org/2015/10/building-rtl-aware-web-apps-websites-part-2/>`__ + +Credits +~~~~~~~ + +Google's `Material Design guide for +RTL <https://material.io/design/usability/bidirectionality.html>`__ diff --git a/docs/code-quality/coding-style/svg_guidelines.rst b/docs/code-quality/coding-style/svg_guidelines.rst new file mode 100644 index 0000000000..f34317c91a --- /dev/null +++ b/docs/code-quality/coding-style/svg_guidelines.rst @@ -0,0 +1,347 @@ +SVG Guidelines +============== + +Pros and cons of SVG for images +------------------------------- + +When used as a document format there is usually a compelling reason that +makes SVG the only solution. When used as an `image +format <https://developer.mozilla.org/en-US/docs/Web/SVG/SVG_as_an_Image>`__, +it is sometimes less obvious whether it would be best to use SVG or a +raster image format for any given image. The vector format SVG and +raster formats like PNG both have their place. When choosing whether or +not to use SVG it is best to understand the advantages and disadvantages +of both. + +File size + Whether SVG or a raster format will produce a smaller file for a + given image depends very much on the image. For example, consider an + image of a path with a gradient fill. The size of an SVG of this + image will be the same regardless of the dimensions of the image. On + the other hand the size of a raster file of the same image will + likely vary tremendously depending on the dimensions of the image + since the larger the dimensions the more pixel data the file needs to + store. At very small dimensions (the extreme case being 1px x 1px) + the raster file will likely be much smaller than the SVG file since + it only needs to store one pixel of data. At large dimensions the + raster file may be much larger than the SVG file. +Scalability, with caveats + One of the primary advantages of SVG is that as it is scaled it does + not pixelate. However, this is not to say that it always does away + with the need to have a collection of raster images for display at + different scales. This can be particularly true for icons. While SVG + may scale well enough for flat-ish icons without a lot of detail, for + icons that try to pack in a lot of detail graphic artists generally + `want to be able to pixel + tweak <https://www.pushing-pixels.org/2011/11/04/about-those-vector-icons.html>`__. +Performance + While SVG provides a lot of flexibility in terms of scaling, + themability, etc. this flexibility depends on doing computations for + SVG images at the time they're displayed, rather than at the time the + author creates them. Consider an image that involves some complex + gradients and filters. If saved as a raster image then the work to + rasterize the gradients and filters takes place on the authors + computer before the result is stored in the raster file. This work + doesn't need to be redone when the image is displayed on someone + else's computer. On the other hand, if the image is saved as an SVG + image then all this work needs to be done each time the SVG is + displayed on someone else's computer. This isn't to say that SVG + images are always slower than raster equivalents. In fact it can be + faster to send vector information from an SVG to a user's GPU than it + is to extract raster data from an equivalent raster image. And even + when an SVG image is slower than a raster equivalent, the difference + is usually not noticeable. However, just don't fall into the trap of + thinking that SVGs are faster than equivalent raster images, or vice + versa. Once again, "it depends". + +Authoring guidelines +-------------------- + +A lot of SVG files (particularly those generated by SVG editors) ship +without being cleaned up and can contain a ton of junk that bloats the +file size and slows down rendering. In general the best way to combat +this is to first run SVG files through a linter such as +`svgo <https://github.com/svg/svgo>`__ (see the Tools section below). +However, when authoring SVGs by hand here are some best practices to +help keep them lightweight. These rules are based on some real examples +seen in Mozilla's code. + +Basics +~~~~~~ + +- Two spaces indenting +- No useless whitespaces or line breaks (see below for more details) +- Adding a license header +- Use double quotes + +Whitespace and line breaks +~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Whitespace +^^^^^^^^^^ + +In addition to trailing whitespace at the end of lines, there are a few +more cases more specific to SVGs: + +- Trailing whitespaces in attribute values (usually seen in path + definitions) +- Excessive whitespace in path or polygon points definition + +Whitespace examples +^^^^^^^^^^^^^^^^^^^ + +This path: + +.. code:: html + + <path d=" M5,5 L1,1z "> + +can be cut down to this: + +.. code:: html + + <path d="M5,5 L1,1z"> + +Similarly, this polygon: + +.. code:: html + + <polygon points=" 0,0 4,4 4,0 "/> + +can be cut down to this: + +.. code:: html + + <polygon points="0,0 4,4 4,0"/> + +Line breaks +^^^^^^^^^^^ + +You should only use line breaks for logical separation or if they help +make the file readable. You should avoid line breaks between every +single element or within attribute values. It's recommended to put the +attributes on the same line as their tag names, if possible. You should +also put the shortest attributes first, so they are easier to spot. + +Unused tags and attributes +~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Editor metadata +^^^^^^^^^^^^^^^ + +Vector editors (Inkscape, Adobe Illustrator, Sketch) usually add a bunch +of metadata in SVG files while saving them. Metadata can mean many +things, including: + +- The typical "Created with *editor*" comments +- Non-standard editor specific tags and attributes (``sketch:foo``, + ``illustrator:foo``, ``sopodi:foo``, …) +- The `XML + namespace <https://developer.mozilla.org/en-US/docs/Web/SVG/Namespaces_Crash_Course>`__ + definition that comes with the latter (``xmlns:sketch``, + ``xmlns:sopodi``, …) + +Other metadata +^^^^^^^^^^^^^^ + +In addition to non-standard editor metadata, standard compliant metadata +also exists. Typical examples of this are ``<title>`` and ``<desc>`` +tags. Although this kind of data is supported by the browser, it can +only be displayed when the SVG is opened in a new tab. Plus, in most of +the cases, the filename is quite descriptive So it's recommended to +remove that kind of metadata since it doesn't bring much value. + +You shouldn't include DOCTYPEs in your SVGs either; they are a source of +many issues, and the SVG WG recommends not to include them. See `SVG +Authoring +guidelines <https://jwatt.org/svg/authoring/#doctype-declaration>`__. + +Avoid the use of CDATA sections +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +`CDATA +sections <https://developer.mozilla.org/en-US/docs/Web/API/CDATASection>`__ +are used to avoid parsing some text as HTML. Most of time, CDATA isn't +needed, for example, the content in ``<style>`` tags doesn't need to be +wrapped in a CDATA section as the content inside the tag is already +correctly parsed as CSS. + +Invisible shapes +^^^^^^^^^^^^^^^^ + +There are two kinds of invisible shapes: The off-screen ones and the +uncolored ones. + +The offscreen shapes are hard to spot, even with an automated tool, and +are usually context aware. Those kinds of shapes are visible but off the +`SVG view +box <https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/viewBox>`__. +Here's `an +example <https://hg.mozilla.org/mozilla-central/diff/9fb143f3b36a/browser/themes/shared/heartbeat-star-lit.svg>`__ +of a file with offscreen shapes. + +On the other hand, the uncolored ones are easier to spot, since they +usually come with styles making them invisible. They must meet two +conditions: they must be devoid of any fill (or a transparent one) or +stroke. + +Unused attributes on root ``<svg>`` element +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +The root ``<svg>`` element can also host many useless attributes. Here's +an +`example <https://hg.mozilla.org/mozilla-central/diff/2d38fecce226/browser/components/loop/content/shared/img/icons-10x10.svg>`__ +taking into account the list below: + +- ``version`` +- ``x="0"`` and ``y="0"`` +- ``enable-background`` (unsupported by Gecko and now deprecated by the + Filter Effects specification) +- ``id`` (id on root element has no effect) +- ``xmlns:xlink`` attribute when there are no ``xlink:href`` attributes + used throughout the file +- Other unused `XML + Namespace <https://developer.mozilla.org/en-US/docs/Web/SVG/Namespaces_Crash_Course>`__ + definitions +- ``xml:space`` when there is no text used in the file + +Other +^^^^^ + +- Empty tags, this may be obvious, but those are sometimes found in + SVGs +- Unreferenced ids (usually on gradient stops, but also on shapes or + paths) +- ``clip-rule`` attribute when the element *is not* a descendant of a + ``<clipPath>`` +- ``fill-rule`` attribute when the element *is* a descendant of a + ``<clipPath>`` +- Unreferenced/Unused clip paths, masks or defs + (`example <https://hg.mozilla.org/mozilla-central/diff/2d38fecce226/toolkit/themes/shared/reader/RM-Plus-24x24.svg>`__) + +Styling +~~~~~~~ + +Styling basics +^^^^^^^^^^^^^^ + +- Privilege short lowercase hex for colors +- Don't use excessive precision for numeric values (usually comes from + illustrator) +- Use descriptive IDs +- Avoid inline styles and use class names or SVG attributes + +Styling examples +'''''''''''''''' + +Here are some examples for excessive number precision: + +- 5.000000e-02 → 0.05 (as seen + `here <https://hg.mozilla.org/mozilla-central/diff/2d38fecce226/browser/themes/shared/devtools/images/tool-network.svg#l1.31>`__) +- -3.728928e-10 → 0 (as seen + `here <https://hg.mozilla.org/mozilla-central/diff/2d38fecce226/browser/themes/shared/aboutNetError_alert.svg#l1.12>`__) +- translate(0.000000, -1.000000) → translate(0, -1) (as seen + `here <https://hg.mozilla.org/mozilla-central/diff/2d38fecce226/browser/themes/shared/heartbeat-icon.svg#l1.13>`__) + +As for descriptive IDs: + +- For gradients: SVG_ID1 → gradient1 (as seen + `here <https://hg.mozilla.org/mozilla-central/diff/2d38fecce226/browser/themes/shared/aboutNetError_alert.svg#l1.12>`__) + +Use of class names +^^^^^^^^^^^^^^^^^^ + +- Avoid using a class if that class is only used once in the file +- If that class only sets a fill or a stroke, it's better to set the + fill/stroke directly on the actual shape, instead of introducing a + class just for that shape. You can also use SVG grouping to avoid + duplicating those attributes +- Avoid introducing variants of the same file (color/style variants), + and use sprites instead (with class names) + +Default style values +^^^^^^^^^^^^^^^^^^^^ + +There's usually no need to set the default style value unless you're +overriding a style. Here are some commonly seen examples: + +- ``style="display: none;"`` on ``<defs>`` elements (a ``<defs>`` + element is hidden by default) +- ``type="text/css"`` on ``<style>`` elements +- ``stroke: none`` or ``stroke-width: 0`` + +SVG grouping +~~~~~~~~~~~~ + +Style grouping +^^^^^^^^^^^^^^ + +Group similarly styled shapes under one ``<g>`` tag; this avoids having +to set the same class/styles on many shapes. + +Avoid excessive grouping +^^^^^^^^^^^^^^^^^^^^^^^^ + +Editors can sometimes do excessive grouping while exporting SVGs. This +is due to the way editors work. + +Nested groups +''''''''''''' + +Avoid multiple-level nesting of groups, these make the SVG less +readable. + +Nested transforms +''''''''''''''''' + +Some editors use ``<g>`` tags to do nested transforms, which is usually +not needed. You can avoid this by doing basic algebra, for example: + +.. code:: xml + + <g transform="translate(-62, -310)"><shape transform="translate(60, 308)"/></g> + +can be cut down to: + +.. code:: xml + + <shape transform="translate(-2,-2)"/> + +because: -62+60 = -310+308 = -2 + +Performance tips +~~~~~~~~~~~~~~~~ + +These rules are optional, but they help speeding up the SVG. + +- Avoid using a ``<use>`` tag when that ``<use>`` tag is being + referenced only once in the whole file. +- Instead of using CSS/SVG + `transforms <https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/transform>`__, + apply directly the transform on the path/shape definition. + +Tools +~~~~~ + +Tools can help to clean SVG files. Note, however that some of the rules +stated above can be hard to detect with automated tools since they +require too much context-awareness. To this date, there doesn't seem to +be a tool that handles all of the above. However, there are some +utilities that cover parts of this document: + +- Mostly complete command line tool: https://github.com/svg/svgo +- Alternatives to SVGO: + + - https://github.com/RazrFalcon/svgcleaner + - https://github.com/scour-project/scour + +- GUI for command line tool (use with "Prettify code" and "Remove + ``<title>``" options on): https://jakearchibald.github.io/svgomg/ +- Good alternative to SVGO/SVGOMG: + https://petercollingridge.appspot.com/svg-editor +- Fixes the excessive number precision: + https://simon.html5.org/tools/js/svg-optimizer/ +- Converts inline styles to SVG + attributes: https://www.w3.org/wiki/SvgTidy +- RaphaelJS has a couple of utilities that may be useful: + `raphael.js <https://dmitrybaranovskiy.github.io/raphael/>`__ diff --git a/docs/code-quality/coding-style/using_cxx_in_firefox_code.rst b/docs/code-quality/coding-style/using_cxx_in_firefox_code.rst new file mode 100644 index 0000000000..3a225fded9 --- /dev/null +++ b/docs/code-quality/coding-style/using_cxx_in_firefox_code.rst @@ -0,0 +1,1075 @@ +Using C++ in Mozilla code +========================= + +C++ language features +--------------------- + +Mozilla code only uses a subset of C++. Runtime type information (RTTI) +is disabled, as it tends to cause a very large increase in codesize. +This means that ``dynamic_cast``, ``typeid()`` and ``<typeinfo>`` cannot +be used in Mozilla code. Also disabled are exceptions; do not use +``try``/``catch`` or throw any exceptions. Libraries that throw +exceptions may be used if you are willing to have the throw instead be +treated as an abort. + +On the side of extending C++, we compile with ``-fno-strict-aliasing``. +This means that when reinterpreting a pointer as a differently-typed +pointer, you don't need to adhere to the "effective type" (of the +pointee) rule from the standard (aka. "the strict aliasing rule") when +dereferencing the reinterpreted pointer. You still need make sure that +you don't violate alignment requirements and need to make sure that the +data at the memory location pointed to forms a valid value when +interpreted according to the type of the pointer when dereferencing the +pointer for reading. Likewise, if you write by dereferencing the +reinterpreted pointer and the originally-typed pointer might still be +dereferenced for reading, you need to make sure that the values you +write are valid according to the original type. This value validity +issue is moot for e.g. primitive integers for which all bit patterns of +their size are valid values. + +- As of Mozilla 59, C++14 mode is required to build Mozilla. +- As of Mozilla 67, MSVC can no longer be used to build Mozilla. +- As of Mozilla 73, C++17 mode is required to build Mozilla. + +This means that C++17 can be used where supported on all platforms. The +list of acceptable features is given below: + +.. list-table:: + :widths: 25 25 25 25 + :header-rows: 3 + + * - + - GCC + - Clang + - + * - Current minimal requirement + - 8.1 + - 8.0 + - + * - Feature + - GCC + - Clang + - Can be used in code + * - ``type_t &&`` + - 4.3 + - 2.9 + - Yes (see notes) + * - ref qualifiers on methods + - 4.8.1 + - 2.9 + - Yes + * - default member-initializers (except for bit-fields) + - 4.7 + - 3.0 + - Yes + * - default member-initializers (for bit-fields) + - 8 + - 6 + - **No** + * - variadic templates + - 4.3 + - 2.9 + - Yes + * - Initializer lists + - 4.4 + - 3.1 + - Yes + * - ``static_assert`` + - 4.3 + - 2.9 + - Yes + * - ``auto`` + - 4.4 + - 2.9 + - Yes + * - lambdas + - 4.5 + - 3.1 + - Yes + * - ``decltype`` + - 4.3 + - 2.9 + - Yes + * - ``Foo<Bar<T>>`` + - 4.3 + - 2.9 + - Yes + * - ``auto func() -> int`` + - 4.4 + - 3.1 + - Yes + * - Templated aliasing + - 4.7 + - 3.0 + - Yes + * - ``nullptr`` + - 4.6 + - 3.0 + - Yes + * - ``enum foo : int16_t`` {}; + - 4.4 + - 2.9 + - Yes + * - ``enum class foo {}``; + - 4.4 + - 2.9 + - Yes + * - ``enum foo;`` + - 4.6 + - 3.1 + - Yes + * - ``[[attributes]]`` + - 4.8 + - 3.3 + - **No** (see notes) + * - ``constexpr`` + - 4.6 + - 3.1 + - Yes + * - ``alignas`` + - 4.8 + - 3.3 + - Yes + * - ``alignof`` + - 4.8 + - 3.3 + - Yes, but see notes ; only clang 3.6 claims as_feature(cxx_alignof) + * - Delegated constructors + - 4.7 + - 3.0 + - Yes + * - Inherited constructors + - 4.8 + - 3.3 + - Yes + * - ``explicit operator bool()`` + - 4.5 + - 3.0 + - Yes + * - ``char16_t/u"string"`` + - 4.4 + - 3.0 + - Yes + * - ``R"(string)"`` + - 4.5 + - 3.0 + - Yes + * - ``operator""()`` + - 4.7 + - 3.1 + - Yes + * - ``=delete`` + - 4.4 + - 2.9 + - Yes + * - ``=default`` + - 4.4 + - 3.0 + - Yes + * - unrestricted unions + - 4.6 + - 3.1 + - Yes + * - ``for (auto x : vec)`` (`be careful about the type of the iterator <https://stackoverflow.com/questions/15176104/c11-range-based-loop-get-item-by-value-or-reference-to-const>`__) + - 4.6 + - 3.0 + - Yes + * - ``override``/``final`` + - 4.7 + - 3.0 + - Yes + * - ``thread_local`` + - 4.8 + - 3.3 + - **No** (see notes) + * - function template default arguments + - 4.3 + - 2.9 + - Yes + * - local structs as template parameters + - 4.5 + - 2.9 + - Yes + * - extended friend declarations + - 4.7 + - 2.9 + - Yes + * - ``0b100`` (C++14) + - 4.9 + - 2.9 + - Yes + * - `Tweaks to some C++ contextual conversions` (C++14) + - 4.9 + - 3.4 + - Yes + * - Return type deduction (C++14) + - 4.9 + - 3.4 + - Yes (but only in template code when you would have used ``decltype (complex-expression)``) + * - Generic lambdas (C++14) + - 4.9 + - 3.4 + - Yes + * - Initialized lambda captures (C++14) + - 4.9 + - 3.4 + - Yes + * - Digit separator (C++14) + - 4.9 + - 3.4 + - Yes + * - Variable templates (C++14) + - 5.0 + - 3.4 + - Yes + * - Relaxed constexpr (C++14) + - 5.0 + - 3.4 + - Yes + * - Aggregate member initialization (C++14) + - 5.0 + - 3.3 + - Yes + * - Clarifying memory allocation (C++14) + - 5.0 + - 3.4 + - Yes + * - [[deprecated]] attribute (C++14) + - 4.9 + - 3.4 + - **No** (see notes) + * - Sized deallocation (C++14) + - 5.0 + - 3.4 + - **No** (see notes) + * - Concepts (Concepts TS) + - 6.0 + - — + - **No** + * - Inline variables (C++17) + - 7.0 + - 3.9 + - Yes + * - constexpr_if (C++17) + - 7.0 + - 3.9 + - Yes + * - constexpr lambdas (C++17) + - — + - — + - **No** + * - Structured bindings (C++17) + - 7.0 + - 4.0 + - Yes + * - Separated declaration and condition in ``if``, ``switch`` (C++17) + - 7.0 + - 3.9 + - Yes + * - `Fold expressions <https://en.cppreference.com/w/cpp/language/fold>`__ (C++17) + - 6.0 + - 3.9 + - Yes + * - [[fallthrough]], [[maybe_unused]], [[nodiscard]] (C++17) + - 7.0 + - 3.9 + - Yes + * - Aligned allocation/deallocation (C++17) + - 7.0 + - 4.0 + - **No** (see notes) + * - Designated initializers (C++20) + - 8.0 (4.7) + - 10.0 (3.0) + - Yes [*sic*] (see notes) + * - #pragma once + - 3.4 + - Yes + - **Not** until we `normalize headers <https://groups.google.com/d/msg/mozilla.dev.platform/PgDjWw3xp8k/eqCFlP4Kz1MJ>`__ + * - `Source code information capture <https://en.cppreference.com/w/cpp/experimental/lib_extensions_2#Source_code_information_capture>`__ + - 8.0 + - — + - **No** + +Sources +~~~~~~~ + +* GCC: https://gcc.gnu.org/projects/cxx-status.html +* Clang: https://clang.llvm.org/cxx_status.html + +Notes +~~~~~ + +rvalue references + Implicit move method generation cannot be used. + +Attributes + Several common attributes are defined in + `mozilla/Attributes.h <https://searchfox.org/mozilla-central/source/mfbt/Attributes.h>`__ + or nscore.h. + +Alignment + Some alignment utilities are defined in `mozilla/Alignment.h + <https://searchfox.org/mozilla-central/source/mfbt/Alignment.h>`__. + + .. caution:: + ``MOZ_ALIGNOF`` and ``alignof`` don't have the same semantics. Be careful of what you + expect from them. + +``[[deprecated]]`` + If we have deprecated code, we should be removing it rather than marking it as + such. Marking things as ``[[deprecated]]`` also means the compiler will warn + if you use the deprecated API, which turns into a fatal error in our + automation builds, which is not helpful. + +Sized deallocation + Our compilers all support this (custom flags are required for GCC and Clang), + but turning it on breaks some classes' ``operator new`` methods, and `some + work <https://bugzilla.mozilla.org/show_bug.cgi?id=1250998>`__ would need to + be done to make it an efficiency win with our custom memory allocator. + +Aligned allocation/deallocation + Our custom memory allocator doesn't have support for these functions. + +Thread locals + ``thread_local`` is not supported on Android. + +Designated initializers + Despite their late addition to C++ (and lack of *official* support by + compilers until relatively recently), `C++20's designated initializers + <https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0329r4.pdf>`__ are + merely a subset of `a feature originally introduced in C99 + <https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html>`__ -- and this + subset has been accepted without comment in C++ code since at least GCC 4.7 + and Clang 3.0. + + +C++ and Mozilla standard libraries +---------------------------------- + +The Mozilla codebase contains within it several subprojects which follow +different rules for which libraries can and can't be used it. The rules +listed here apply to normal platform code, and assume unrestricted +usability of MFBT or XPCOM APIs. + +.. warning:: + + The rest of this section is a draft for expository and exploratory + purposes. Do not trust the information listed here. + +What follows is a list of standard library components provided by +Mozilla or the C++ standard. If an API is not listed here, then it is +not permissible to use it in Mozilla code. Deprecated APIs are not +listed here. In general, prefer Mozilla variants of data structures to +standard C++ ones, even when permitted to use the latter, since Mozilla +variants tend to have features not found in the standard library (e.g., +memory size tracking) or have more controllable performance +characteristics. + +A list of approved standard library headers is maintained in +`config/stl-headers.mozbuild <https://searchfox.org/mozilla-central/source/config/stl-headers.mozbuild>`__. + + +Data structures +~~~~~~~~~~~~~~~ + +.. list-table:: + :widths: 25 25 25 25 + :header-rows: 1 + + * - Name + - Header + - STL equivalent + - Notes + * - ``nsAutoTArray`` + - ``nsTArray.h`` + - + - Like ``nsTArray``, but will store a small amount as stack storage + * - ``nsAutoTObserverArray`` + - ``nsTObserverArray.h`` + - + - Like ``nsTObserverArray``, but will store a small amount as stack storage + * - ``mozilla::BloomFilter`` + - ``mozilla/BloomFilter.h`` + - + - Probabilistic set membership (see `Wikipedia <https://en.wikipedia.org/wiki/Bloom_filter#Counting_filters>`__) + * - ``nsClassHashtable`` + - ``nsClassHashtable.h`` + - + - Adaptation of nsTHashtable, see :ref:`XPCOM Hashtable Guide` + * - ``nsCOMArray`` + - ``nsCOMArray.h`` + - + - Like ``nsTArray<nsCOMPtr<T>>`` + * - ``nsDataHashtable`` + - ``nsClassHashtable.h`` + - ``std::unordered_map`` + - Adaptation of ``nsTHashtable``, see :ref:`XPCOM Hashtable Guide` + * - ``nsDeque`` + - ``nsDeque.h`` + - ``std::deque<void *>`` + - + * - ``mozilla::EnumSet`` + - ``mozilla/EnumSet.h`` + - + - Like ``std::set``, but for enum classes. + * - ``mozilla::Hash{Map,Set}`` + - `mozilla/HashTable.h <https://searchfox.org/mozilla-central/source/mfbt/HashTable.h>`__ + - ``std::unordered_{map,set}`` + - A general purpose hash map and hash set. + * - ``nsInterfaceHashtable`` + - ``nsInterfaceHashtable.h`` + - ``std::unordered_map`` + - Adaptation of ``nsTHashtable``, see :ref:`XPCOM Hashtable Guide` + * - ``mozilla::LinkedList`` + - ``mozilla/LinkedList.h`` + - ``std::list`` + - Doubly-linked list + * - ``nsRef PtrHashtable`` + - ``nsRefPtrHashtable.h`` + - ``std::unordered_map`` + - Adaptation of ``nsTHashtable``, see :ref:`XPCOM Hashtable Guide` + * - ``mozilla::SegmentedVector`` + - ``mozilla/SegmentedVector.h`` + - ``std::deque`` w/o O(1) pop_front + - Doubly-linked list of vector elements + * - ``mozilla::SplayTree`` + - ``mozilla/SplayTree.h`` + - + - Quick access to recently-accessed elements (see `Wikipedia <https://en.wikipedia.org/wiki/Splay_tree>`__) + * - ``nsTArray`` + - ``nsTArray.h`` + - ``std::vector`` + - + * - ``nsTHashtable`` + - ``nsTHashtable.h`` + - ``std::unordered_{map,set}`` + - See :ref:`XPCOM Hashtable Guide`, you probably want a subclass + * - ``nsTObserverArray`` + - ``nsTObserverArray.h`` + - + - Like ``nsTArray``, but iteration is stable even through mutation + * - ``nsTPriorityQueue`` + - ``nsTPriorityQueue.h`` + - ``std::priority_queue`` + - Unlike the STL class, not a container adapter + * - ``mozilla::Vector`` + - ``mozilla/Vector.h`` + - ``std::vector`` + - + * - ``mozilla::Buffer`` + - ``mozilla/Buffer.h`` + - + - Unlike ``Array``, has a run-time variable length. Unlike ``Vector``, does not have capacity and growth mechanism. Unlike ``Span``, owns its buffer. + + +Safety utilities +~~~~~~~~~~~~~~~~ + +.. list-table:: + :widths: 25 25 25 25 + :header-rows: 1 + + * - Name + - Header + - STL equivalent + - Notes + * - ``mozilla::Array`` + - ``mfbt/Array.h`` + - + - safe array index + * - ``mozilla::AssertedCast`` + - ``mfbt/Casting.h`` + - + - casts + * - ``mozilla::CheckedInt`` + - ``mfbt/CheckedInt.h`` + - + - avoids overflow + * - ``nsCOMPtr`` + - ``xpcom/base/nsCOMPtr.h`` + - ``std::shared_ptr`` + - + * - ``mozilla::EnumeratedArray`` + - ``mfbt/EnumeratedArray.h`` + - ``mozilla::Array`` + - + * - ``mozilla::Maybe`` + - ``mfbt/Maybe.h`` + - ``std::optional`` + - + * - ``mozilla::RangedPtr`` + - ``mfbt/RangedPtr.h`` + - + - like ``mozilla::Span`` but with two pointers instead of pointer and length + * - ``mozilla::RefPtr`` + - ``mfbt/RefPtr.h`` + - ``std::shared_ptr`` + - + * - ``mozilla::Span`` + - ``mozilla/Span.h`` + - ``gsl::span``, ``absl::Span``, ``std::string_view``, ``std::u16string_view`` + - Rust's slice concept for C++ (without borrow checking) + * - ``StaticRefPtr`` + - ``xpcom/base/StaticPtr.h`` + - + - ``nsRefPtr`` w/o static constructor + * - ``mozilla::UniquePtr`` + - ``mfbt/UniquePtr.h`` + - ``std::unique_ptr`` + - + * - ``mozilla::WeakPtr`` + - ``mfbt/WeakPtr.h`` + - ``std::weak_ptr`` + - + * - ``nsWeakPtr`` + - ``xpcom/base/nsWeakPtr.h`` + - ``std::weak_ptr`` + - + + +Strings +~~~~~~~ + +See the :doc:`Mozilla internal string guide </xpcom/stringguide>` for +usage of ``nsAString`` (our copy-on-write replacement for +``std::u16string``) and ``nsACString`` (our copy-on-write replacement +for ``std::string``). + +Be sure not to introduce further uses of ``std::wstring``, which is not +portable! (Some uses exist in the IPC code.) + + +Algorithms +~~~~~~~~~~ + +.. list-table:: + :widths: 25 25 + + * - ``mozilla::BinarySearch`` + - ``mfbt/BinarySearch.h`` + * - ``mozilla::BitwiseCast`` + - ``mfbt/Casting.h`` (strict aliasing-safe cast) + * - ``mozilla/MathAlgorithms.h`` + - (rotate, ctlz, popcount, gcd, abs, lcm) + * - ``mozilla::RollingMean`` + - ``mfbt/RollingMean.h`` () + + +Concurrency +~~~~~~~~~~~ + +.. list-table:: + :widths: 25 25 25 25 + :header-rows: 1 + + * - Name + - Header + - STL/boost equivalent + - Notes + * - ``mozilla::Atomic`` + - mfbt/Atomic.h + - ``std::atomic`` + - + * - ``mozilla::CondVar`` + - xpcom/threads/CondVar.h + - ``std::condition_variable`` + - + * - ``mozilla::DataMutex`` + - xpcom/threads/DataMutex.h + - ``boost::synchronized_value`` + - + * - ``mozilla::Monitor`` + - xpcom/threads/Monitor.h + - + - + * - ``mozilla::Mutex`` + - xpcom/threads/Mutex.h + - ``std::mutex`` + - + * - ``mozilla::ReentrantMonitor`` + - xpcom/threads/ReentrantMonitor.h + - + - + * - ``mozilla::StaticMutex`` + - xpcom/base/StaticMutex.h + - ``std::mutex`` + - Mutex that can (and in fact, must) be used as a global/static variable. + + +Miscellaneous +~~~~~~~~~~~~~ + +.. list-table:: + :widths: 25 25 25 25 + :header-rows: 1 + + * - Name + - Header + - STL/boost equivalent + - Notes + * - ``mozilla::AlignedStorage`` + - mfbt/Alignment.h + - ``std::aligned_storage`` + - + * - ``mozilla::MaybeOneOf`` + - mfbt/MaybeOneOf.h + - ``std::optional<std::variant<T1, T2>>`` + - ~ ``mozilla::Maybe<union {T1, T2}>`` + * - ``mozilla::Pair`` + - mfbt/Pair.h + - ``std::tuple<T1, T2>`` + - minimal space! + * - ``mozilla::TimeStamp`` + - xpcom/ds/TimeStamp.h + - ``std::chrono::time_point`` + - + * - + - mozilla/PodOperations.h + - + - C++ versions of ``memset``, ``memcpy``, etc. + * - + - mozilla/ArrayUtils.h + - + - + * - + - mozilla/Compression.h + - + - + * - + - mozilla/Endian.h + - + - + * - + - mozilla/FloatingPoint.h + - + - + * - + - mozilla/HashFunctions.h + - ``std::hash`` + - + * - + - mozilla/Move.h + - ``std::move``, ``std::swap``, ``std::forward`` + - + + +Mozilla data structures and standard C++ ranges and iterators +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Some Mozilla-defined data structures provide STL-style +`iterators <https://en.cppreference.com/w/cpp/named_req/Iterator>`__ and +are usable in `range-based for +loops <https://en.cppreference.com/w/cpp/language/range-for>`__ as well +as STL `algorithms <https://en.cppreference.com/w/cpp/algorithm>`__. + +Currently, these include: + +.. list-table:: + :widths: 16 16 16 16 16 + :header-rows: 1 + + * - Name + - Header + - Bug(s) + - Iterator category + - Notes + * - ``nsTArray`` + - ``xpcom/ds/n sTArray.h`` + - `1126552 <https://bugzilla.mozilla.org/show_bug.cgi?id=1126552>`__ + - Random-access + - Also reverse-iterable. Also supports remove-erase pattern via RemoveElementsAt method. Also supports back-inserting output iterators via ``MakeBackInserter`` function. + * - ``nsBaseHashtable`` and subclasses: ``nsClassHashtable`` ``nsDataHashtable`` ``nsInterfaceHashtable`` ``nsJSThingHashtable`` ``nsRefPtrHashtable`` + - ``xpcom/ds/nsBaseHashtable.h`` ``xpcom/ds/nsClassHashtable.h`` ``xpcom/ds/nsDataHashtable.h`` ``xpcom/ds/nsInterfaceHashtable.h`` ``xpcom/ds/nsJSThingHashtable.h`` ``xpcom/ds/nsRefPtrHashtable.h`` + - `1575479 <https://bugzilla.mozilla.org/show_bug.cgi?id=1575479>`__ + - Forward + - + * - ``nsCOMArray`` + - ``xpcom/ds/nsCOMArray.h`` + - `1342303 <https://bugzilla.mozilla.org/show_bug.cgi?id=1342303>`__ + - Random-access + - Also reverse-iterable. + * - ``Array`` ``EnumerationArray`` ``RangedArray`` + - ``mfbt/Array.h`` ``mfbt/EnumerationArray.h`` ``mfbt/RangedArray.h`` + - `1216041 <https://bugzilla.mozilla.org/show_bug.cgi?id=1216041>`__ + - Random-access + - Also reverse-iterable. + * - ``Buffer`` + - ``mfbt/Buffer.h`` + - `1512155 <https://bugzilla.mozilla.org/show_bug.cgi?id=1512155>`__ + - Random-access + - Also reverse-iterable. + * - ``DoublyLinkedList`` + - ``mfbt/DoublyLinkedList.h`` + - `1277725 <https://bugzilla.mozilla.org/show_bug.cgi?id=1277725>`__ + - Forward + - + * - ``EnumeratedRange`` + - ``mfbt/EnumeratedRange.h`` + - `1142999 <https://bugzilla.mozilla.org/show_bug.cgi?id=1142999>`__ + - *Missing* + - Also reverse-iterable. + * - ``IntegerRange`` + - ``mfbt/IntegerRange.h`` + - `1126701 <https://bugzilla.mozilla.org/show_bug.cgi?id=1126701>`__ + - *Missing* + - Also reverse-iterable. + * - ``SmallPointerArray`` + - ``mfbt/SmallPointerArray.h`` + - `1331718 <https://bugzilla.mozilla.org/show_bug.cgi?id=1331718>`__ + - Random-access + - + * - ``Span`` + - ``mfbt/Span.h`` + - `1295611 <https://bugzilla.mozilla.org/show_bug.cgi?id=1295611>`__ + - Random-access + - Also reverse-iterable. + +Note that if the iterator category is stated as "missing", the type is +probably only usable in range-based for. This is most likely just an +omission, which could be easily fixed. + +Useful in this context are also the class template ``IteratorRange`` +(which can be used to construct a range from any pair of iterators) and +function template ``Reversed`` (which can be used to reverse any range), +both defined in ``mfbt/ReverseIterator.h`` + + +Further C++ rules +----------------- + + +Don't use static constructors +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +(You probably shouldn't be using global variables to begin with. Quite +apart from the weighty software-engineering arguments against them, +globals affect startup time! But sometimes we have to do ugly things.) + +Non-portable example: + +.. code-block:: cpp + + FooBarClass static_object(87, 92); + + void + bar() + { + if (static_object.count > 15) { + ... + } + } + +Once upon a time, there were compiler bugs that could result in +constructors not being called for global objects. Those bugs are +probably long gone by now, but even with the feature working correctly, +there are so many problems with correctly ordering C++ constructors that +it's easier to just have an init function: + +.. code-block:: cpp + + static FooBarClass* static_object; + + FooBarClass* + getStaticObject() + { + if (!static_object) + static_object = + new FooBarClass(87, 92); + return static_object; + } + + void + bar() + { + if (getStaticObject()->count > 15) { + ... + } + } + + +Don't use exceptions +~~~~~~~~~~~~~~~~~~~~ + +See the introduction to the "C++ language features" section at the start +of this document. + + +Don't use Run-time Type Information +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +See the introduction to the "C++ language features" section at the start +of this document. + +If you need runtime typing, you can achieve a similar result by adding a +``classOf()`` virtual member function to the base class of your +hierarchy and overriding that member function in each subclass. If +``classOf()`` returns a unique value for each class in the hierarchy, +you'll be able to do type comparisons at runtime. + + +Don't use the C++ standard library (including iostream and locale) +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +See the section "C++ and Mozilla standard libraries". + + +Use C++ lambdas, but with care +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +C++ lambdas are supported across all our compilers now. Rejoice! We +recommend explicitly listing out the variables that you capture in the +lambda, both for documentation purposes, and to double-check that you're +only capturing what you expect to capture. + + +Use namespaces +~~~~~~~~~~~~~~ + +Namespaces may be used according to the style guidelines in :ref:`C++ Coding style`. + + +Don't mix varargs and inlines +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +What? Why are you using varargs to begin with?! Stop that at once! + + +Make header files compatible with C and C++ +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Non-portable example: + +.. code-block:: cpp + + /*oldCheader.h*/ + int existingCfunction(char*); + int anotherExistingCfunction(char*); + + /* oldCfile.c */ + #include "oldCheader.h" + ... + + // new file.cpp + extern "C" { + #include "oldCheader.h" + }; + ... + +If you make new header files with exposed C interfaces, make the header +files work correctly when they are included by both C and C++ files. + +(If you need to include a C header in new C++ files, that should just +work. If not, it's the C header maintainer's fault, so fix the header if +you can, and if not, whatever hack you come up with will probably be +fine.) + +Portable example: + +.. code-block:: cpp + + /* oldCheader.h*/ + PR_BEGIN_EXTERN_C + int existingCfunction(char*); + int anotherExistingCfunction(char*); + PR_END_EXTERN_C + + /* oldCfile.c */ + #include "oldCheader.h" + ... + + // new file.cpp + #include "oldCheader.h" + ... + +There are number of reasons for doing this, other than just good style. +For one thing, you are making life easier for everyone else, doing the +work in one common place (the header file) instead of all the C++ files +that include it. Also, by making the C header safe for C++, you document +that "hey, this file is now being included in C++". That's a good thing. +You also avoid a big portability nightmare that is nasty to fix... + + +Use override on subclass virtual member functions +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +The ``override`` keyword is supported in C++11 and in all our supported +compilers, and it catches bugs. + + +Always declare a copy constructor and assignment operator +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Many classes shouldn't be copied or assigned. If you're writing one of +these, the way to enforce your policy is to declare a deleted copy +constructor as private and not supply a definition. While you're at it, +do the same for the assignment operator used for assignment of objects +of the same class. Example: + +.. code-block:: cpp + + class Foo { + ... + private: + Foo(const Foo& x) = delete; + Foo& operator=(const Foo& x) = delete; + }; + +Any code that implicitly calls the copy constructor will hit a +compile-time error. That way nothing happens in the dark. When a user's +code won't compile, they'll see that they were passing by value, when +they meant to pass by reference (oops). + + +Be careful of overloaded methods with like signatures +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +It's best to avoid overloading methods when the type signature of the +methods differs only by one "abstract" type (e.g. ``PR_Int32`` or +``int32``). What you will find as you move that code to different +platforms, is suddenly on the Foo2000 compiler your overloaded methods +will have the same type-signature. + + +Type scalar constants to avoid unexpected ambiguities +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Non-portable code: + +.. code-block:: cpp + + class FooClass { + // having such similar signatures + // is a bad idea in the first place. + void doit(long); + void doit(short); + }; + + void + B::foo(FooClass* xyz) + { + xyz->doit(45); + } + +Be sure to type your scalar constants, e.g., ``uint32_t(10)`` or +``10L``. Otherwise, you can produce ambiguous function calls which +potentially could resolve to multiple methods, particularly if you +haven't followed (2) above. Not all of the compilers will flag ambiguous +method calls. + +Portable code: + +.. code-block:: cpp + + class FooClass { + // having such similar signatures + // is a bad idea in the first place. + void doit(long); + void doit(short); + }; + + void + B::foo(FooClass* xyz) + { + xyz->doit(45L); + } + + +Use nsCOMPtr in XPCOM code +~~~~~~~~~~~~~~~~~~~~~~~~~~ + +See the ``nsCOMPtr`` `User +Manual <https://developer.mozilla.org/en-US/docs/Using_nsCOMPtr>`__ for +usage details. + + +Don't use identifiers that start with an underscore +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +This rule occasionally surprises people who've been hacking C++ for +decades. But it comes directly from the C++ standard! + +According to the C++ Standard, 17.4.3.1.2 Global Names +[lib.global.names], paragraph 1: + +Certain sets of names and function signatures are always reserved to the +implementation: + +- Each name that contains a double underscore (__) or begins with an + underscore followed by an uppercase letter (2.11) is reserved to the + implementation for any use. +- **Each name that begins with an underscore is reserved to the + implementation** for use as a name in the global namespace. + + +Stuff that is good to do for C or C++ +------------------------------------- + + +Avoid conditional #includes when possible +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Don't write an ``#include`` inside an ``#ifdef`` if you could instead +put it outside. Unconditional includes are better because they make the +compilation more similar across all platforms and configurations, so +you're less likely to cause stupid compiler errors on someone else's +favorite platform that you never use. + +Bad code example: + +.. code-block:: cpp + + #ifdef MOZ_ENABLE_JPEG_FOUR_BILLION + #include <stdlib.h> // <--- don't do this + #include "jpeg4e9.h" // <--- only do this if the header really might not be there + #endif + +Of course when you're including different system files for different +machines, you don't have much choice. That's different. + + +Every .cpp source file should have a unique name +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Every object file linked into libxul needs to have a unique name. Avoid +generic names like nsModule.cpp and instead use nsPlacesModule.cpp. + + +Turn on warnings for your compiler, and then write warning free code +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +What generates a warning on one platform will generate errors on +another. Turn warnings on. Write warning-free code. It's good for you. +Treat warnings as errors by adding +``ac_add_options --enable-warnings-as-errors`` to your mozconfig file. + + +Use the same type for all bitfields in a ``struct`` or ``class`` +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Some compilers do not pack the bits when different bitfields are given +different types. For example, the following struct might have a size of +8 bytes, even though it would fit in 1: + +.. code-block:: cpp + + struct { + char ch: 1; + int i: 1; + }; + + +Don't use an enum type for a bitfield +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +The classic example of this is using ``PRBool`` for a boolean bitfield. +Don't do that. ``PRBool`` is a signed integer type, so the bitfield's +value when set will be ``-1`` instead of ``+1``, which---I know, +*crazy*, right? The things C++ hackers used to have to put up with... + +You shouldn't be using ``PRBool`` anyway. Use ``bool``. Bitfields of +type ``bool`` are fine. + +Enums are signed on some platforms (in some configurations) and unsigned +on others and therefore unsuitable for writing portable code when every +bit counts, even if they happen to work on your system. diff --git a/docs/code-quality/index.rst b/docs/code-quality/index.rst new file mode 100644 index 0000000000..b664f221be --- /dev/null +++ b/docs/code-quality/index.rst @@ -0,0 +1,180 @@ +Code quality +============ + +Because Firefox is a complex piece of software, a lot of tools are +executed to identify issues at development phase. +In this document, we try to list these all tools. + + +.. toctree:: + :maxdepth: 1 + :glob: + + static-analysis/index.rst + lint/index.rst + coding-style/index.rst + +.. list-table:: C/C++ + :header-rows: 1 + :widths: 20 20 20 20 20 + + * - Tools + - Has autofixes + - Meta bug + - More info + - Upstream + * - Custom clang checker + - + - + - `Source <https://searchfox.org/mozilla-central/source/build/clang-plugin>`_ + - + * - Clang-Tidy + - Yes + - `bug 712350 <https://bugzilla.mozilla.org/show_bug.cgi?id=712350>`__ + - :ref:`Static analysis <Static Analysis>` + - https://clang.llvm.org/extra/clang-tidy/checks/list.html + * - Clang analyzer + - + - `bug 712350 <https://bugzilla.mozilla.org/show_bug.cgi?id=712350>`__ + - + - https://clang-analyzer.llvm.org/ + * - cpp virtual final + - + - + - :ref:`cpp virtual final` + - + * - clang-format + - Yes + - `bug 1188202 <https://bugzilla.mozilla.org/show_bug.cgi?id=1188202>`__ + - :ref:`Formatting C++ Code With clang-format` + - https://clang.llvm.org/docs/ClangFormat.html + +.. list-table:: CSS + :widths: 20 20 20 20 20 + :header-rows: 1 + + * - Tools + - Has autofixes + - Meta bug + - More info + - Upstream + * - Stylelint + - Yes + - `bug 1762027 <https://bugzilla.mozilla.org/show_bug.cgi?id=1762027>`__ + - :ref:`Stylelint` + - https://stylelint.io/ + +.. list-table:: JavaScript + :widths: 20 20 20 20 20 + :header-rows: 1 + + * - Tools + - Has autofixes + - Meta bug + - More info + - Upstream + * - Eslint + - Yes + - `bug 1229856 <https://bugzilla.mozilla.org/show_bug.cgi?id=1229856>`__ + - :ref:`ESLint` + - https://eslint.org/ + * - Mozilla ESLint + - + - `bug 1229856 <https://bugzilla.mozilla.org/show_bug.cgi?id=1229856>`__ + - :ref:`Mozilla ESLint Plugin` + - + * - Prettier + - Yes + - `bug 1558517 <https://bugzilla.mozilla.org/show_bug.cgi?id=1558517>`__ + - :ref:`JavaScript Coding style` + - https://prettier.io/ + +.. list-table:: Python + :widths: 20 20 20 20 20 + :header-rows: 1 + + * - Tools + - Has autofixes + - Meta bug + - More info + - Upstream + * - ruff + - Yes + - `bug 1811850 <https://bugzilla.mozilla.org/show_bug.cgi?id=1811850>`__ + - :ref:`ruff` + - https://github.com/charliermarsh/ruff + * - black + - Yes + - `bug 1555560 <https://bugzilla.mozilla.org/show_bug.cgi?id=1555560>`__ + - :ref:`black` + - https://black.readthedocs.io/en/stable + +.. list-table:: Rust + :widths: 20 20 20 20 20 + :header-rows: 1 + + * - Tools + - Has autofixes + - Meta bug + - More info + - Upstream + * - Rustfmt + - Yes + - `bug 1454764 <https://bugzilla.mozilla.org/show_bug.cgi?id=1454764>`__ + - :ref:`Rustfmt` + - https://github.com/rust-lang/rustfmt + * - Clippy + - + - `bug 1361342 <https://bugzilla.mozilla.org/show_bug.cgi?id=1361342>`__ + - :ref:`clippy` + - https://github.com/rust-lang/rust-clippy + +.. list-table:: Java/Kotlin + :widths: 20 20 20 20 20 + :header-rows: 1 + + * - Tools + - Has autofixes + - Meta bug + - More info + - Upstream + * - Spotless + - Yes + - `bug 1571899 <https://bugzilla.mozilla.org/show_bug.cgi?id=1571899>`__ + - :ref:`Spotless` + - https://github.com/diffplug/spotless + +.. list-table:: Others + :widths: 20 20 20 20 20 + :header-rows: 1 + + * - Tools + - Has autofixes + - Meta bug + - More info + - Upstream + * - shellcheck + - + - + - + - https://www.shellcheck.net/ + * - rstchecker + - + - + - :ref:`RST Linter` + - https://github.com/myint/rstcheck + * - Typo detection + - Yes + - + - :ref:`Codespell` + - https://github.com/codespell-project/codespell + * - Fluent Lint + - No + - + - :ref:`Fluent Lint` + - + * - YAML linter + - No + - + - :ref:`yamllint` + - https://github.com/adrienverge/yamllint diff --git a/docs/code-quality/lint/create.rst b/docs/code-quality/lint/create.rst new file mode 100644 index 0000000000..c4d8b15480 --- /dev/null +++ b/docs/code-quality/lint/create.rst @@ -0,0 +1,373 @@ +Adding a New Linter to the Tree +=============================== + +Linter Requirements +------------------- + +For a linter to be integrated into the mozilla-central tree, it needs to have: + +* Any required dependencies should be installed as part of ``./mach bootstrap`` +* A ``./mach lint`` interface +* Running ``./mach lint`` command must pass (note, linters can be disabled for individual directories) +* Taskcluster/Treeherder integration +* In tree documentation (under ``docs/code-quality/lint``) to give a basic summary, links and any other useful information +* Unit tests (under ``tools/lint/test``) to make sure that the linter works as expected and we don't regress. + +The review group in Phabricator is ``#linter-reviewers``. + +Linter Basics +------------- + +A linter is a yaml file with a ``.yml`` extension. Depending on how the type of linter, there may +be python code alongside the definition, pointed to by the 'payload' attribute. + +Here's a trivial example: + +no-eval.yml + +.. code-block:: yaml + + EvalLinter: + description: Ensures the string eval doesn't show up. + extensions: ['js'] + type: string + payload: eval + +Now ``no-eval.yml`` gets passed into :func:`LintRoller.read`. + + +Linter Types +------------ + +There are four types of linters, though more may be added in the future. + +1. string - fails if substring is found +2. regex - fails if regex matches +3. external - fails if a python function returns a non-empty result list +4. structured_log - fails if a mozlog logger emits any lint_error or lint_warning log messages + +As seen from the example above, string and regex linters are very easy to create, but they +should be avoided if possible. It is much better to use a context aware linter for the language you +are trying to lint. For example, use eslint to lint JavaScript files, use ruff to lint Python +files, etc. + +Which brings us to the third and most interesting type of linter, +external. External linters call an arbitrary python function which is +responsible for not only running the linter, but ensuring the results +are structured properly. For example, an external type could shell out +to a 3rd party linter, collect the output and format it into a list of +:class:`Issue` objects. The signature for this python +function is ``lint(files, config, **kwargs)``, where ``files`` is a list of +files to lint and ``config`` is the linter definition defined in the ``.yml`` +file. + +Structured log linters are much like external linters, but suitable +for cases where the linter code is using mozlog and emits +``lint_error`` or ``lint_warning`` logging messages when the lint +fails. This is recommended for writing novel gecko-specific lints. In +this case the signature for lint functions is ``lint(files, config, logger, +**kwargs)``. + + +Linter Definition +----------------- + +Each ``.yml`` file must have at least one linter defined in it. Here are the supported keys: + +* description - A brief description of the linter's purpose (required) +* type - One of 'string', 'regex' or 'external' (required) +* payload - The actual linting logic, depends on the type (required) +* include - A list of file paths that will be considered (optional) +* exclude - A list of file paths or glob patterns that must not be matched (optional) +* extensions - A list of file extensions to be considered (optional) +* setup - A function that sets up external dependencies (optional) +* support-files - A list of glob patterns matching configuration files (optional) +* find-dotfiles - If set to ``true``, run on dot files (.*) (optional) +* ignore-case - If set to ``true`` and ``type`` is regex, ignore the case (optional) + +In addition to the above, some ``.yml`` files correspond to a single lint rule. For these, the +following additional keys may be specified: + +* message - A string to print on infraction (optional) +* hint - A string with a clue on how to fix the infraction (optional) +* rule - An id string for the lint rule (optional) +* level - The severity of the infraction, either 'error' or 'warning' (optional) + +For structured_log lints the following additional keys apply: + +* logger - A StructuredLog object to use for logging. If not supplied + one will be created (optional) + + +Example +------- + +Here is an example of an external linter that shells out to the Python ruff linter, +let's call the file ``ruff_lint.py`` (`in-tree version <https://searchfox.org/mozilla-central/source/tools/lint/python/ruff.py>`__): + +.. code-block:: python + + import json + import os + import subprocess + from collections import defaultdict + from shutil import which + + from mozlint import result + + + RUFF_NOT_FOUND = """ + Could not find ruff! Install ruff and try again. + """.strip() + + + def lint(paths, config, **lintargs): + binary = which('ruff') + if not binary: + print(RUFF_NOT_FOUND) + return 1 + + + cmd = ["ruff", "check", "--force-exclude", "--format=json"] + paths + output = subprocess.run(cmd, stdout=subprocess.PIPE, env=os.environ).output + + # all passed + if not output: + return [] + + try: + issues = json.loads(output) + except json.JSONDecodeError: + log.error(f"Could not parse output: {output}") + + results = [] + for issue in issues: + # convert ruff's format to mozlint's format + res = { + "path": issue["filename"], + "lineno": issue["location"]["row"], + "column": issue["location"]["column"], + "lineoffset": issue["end_location"]["row"] - issue["location"]["row"], + "message": issue["message"], + "rule": issue["code"], + "level": "error", + } + + if issue["fix"]: + res["hint"] = issue["fix"]["message"] + + results.append(result.from_config(config, **res)) + + return {"results": results, "fixed": fixed} + +Now here is the linter definition that would call it: + +.. code-block:: yaml + + ruff: + description: Python Linter + include: ["."] + extensions: ["py"] + support-files: + - "**/.ruff.toml" + - "**/ruff.toml" + - "**/pyproject.toml" + type: external + payload: py.ruff:lint + +Notice the payload has two parts, delimited by ':'. The first is the module +path, which ``mozlint`` will attempt to import. The second is the object path +within that module (e.g, the name of a function to call). It is up to consumers +of ``mozlint`` to ensure the module is in ``sys.path``. Structured log linters +use the same import mechanism. + +The ``support-files`` key is used to list configuration files or files related +to the running of the linter itself. If using ``--outgoing`` or ``--workdir`` +and one of these files was modified, the entire tree will be linted instead of +just the modified files. + +Result definition +----------------- + +When generating the list of results, the following values are available. + +.. csv-table:: + :header: "Name", "Description", "Optional" + :widths: 20, 40, 10 + + "linter", "Name of the linter that flagged this error", "" + "path", "Path to the file containing the error", "" + "message", "Text describing the error", "" + "lineno", "Line number that contains the error", "" + "column", "Column containing the error", "" + "level", "Severity of the error, either 'warning' or 'error' (default 'error')", "Yes" + "hint", "Suggestion for fixing the error", "Yes" + "source", "Source code context of the error", "Yes" + "rule", "Name of the rule that was violated", "Yes" + "lineoffset", "Denotes an error spans multiple lines, of the form (<lineno offset>, <num lines>)", "Yes" + "diff", "A diff describing the changes that need to be made to the code", "Yes" + + +Automated testing +----------------- + +Every new checker must have associated tests. If your linter is ``mylinter`` then the +test file should be named ``tools/lint/test/test_mylinter.py`` and any example files +named like ``tools/lint/test/files/mylinter/my-example-file``. Be sure that your test +has been added as a section ``["test_mylinter.py"]`` in the manifest ``tools/lint/test/python.toml``. + +They should be pretty easy to write as most of the work is managed by the Mozlint +framework. The key declaration is the ``LINTER`` variable which must match +the linker declaration. + +As an example, the `ruff test <https://searchfox.org/mozilla-central/source/tools/lint/test/test_ruff.py>`_ looks like the following snippet: + +.. code-block:: python + + import mozunit + LINTER = 'ruff' + + def test_lint_ruff(lint, paths): + results = lint(paths('bad.py')) + assert len(results) == 2 + assert results[0].rule == 'F401' + assert results[1].rule == 'E501' + assert results[1].lineno == 5 + + if __name__ == '__main__': + mozunit.main() + +As always with tests, please make sure that enough positive and negative cases +are covered. + +To run the tests: + +.. code-block:: shell + + $ ./mach python-test --subsuite mozlint + +To run a specific test: + +.. code-block:: shell + + ./mach python-test --subsuite mozlint tools/lint/test/test_black.py + +More tests can be `found in-tree <https://searchfox.org/mozilla-central/source/tools/lint/test>`_. + +Tracking fixed issues +--------------------- + +All the linters that provide ``fix support`` returns a dictionary instead of a list. + +``{"results":result,"fixed":fixed}`` + +* results - All the linting errors it was not able to fix +* fixed - Count of fixed errors (for ``fix=False`` this is 0) + +Some linters (example: `codespell <https://searchfox.org/mozilla-central/rev/0379f315c75a2875d716b4f5e1a18bf27188f1e6/tools/lint/spell/__init__.py#145-163>`_) might require two passes to count the number of fixed issues. +Others might just need `some tuning <https://searchfox.org/mozilla-central/rev/0379f315c75a2875d716b4f5e1a18bf27188f1e6/tools/lint/file-whitespace/__init__.py#28,60,85,112>`_. + +For adding tests to check your fixed count, add a global variable ``fixed = 0`` +and write a function to add your test as mentioned under ``Automated testing`` section. + + +Here's an example + +.. code-block:: python + + fixed = 0 + + + def test_lint_codespell_fix(lint, create_temp_file): + # Typo has been fixed in the contents to avoid triggering warning + # 'informations' ----> 'information' + contents = """This is a file with some typos and information. + But also testing false positive like optin (because this isn't always option) + or stuff related to our coding style like: + aparent (aParent). + but detects mistakes like mozilla + """.lstrip() + + path = create_temp_file(contents, "ignore.rst") + lint([path], fix=True) + + assert fixed == 2 + + +Bootstrapping Dependencies +-------------------------- + +Many linters, especially 3rd party ones, will require a set of dependencies. It +could be as simple as installing a binary from a package manager, or as +complicated as pulling a whole graph of tools, plugins and their dependencies. + +Either way, to reduce the burden on users, linters should strive to provide +automated bootstrapping of all their dependencies. To help with this, +``mozlint`` allows linters to define a ``setup`` config, which has the same +path object format as an external payload. For example (`in-tree version <https://searchfox.org/mozilla-central/source/tools/lint/ruff.yml>`__): + +.. code-block:: yaml + + ruff: + description: Python linter + include: ['.'] + extensions: ['py'] + type: external + payload: py.ruff:lint + setup: py.ruff:setup + +The setup function takes a single argument, the root of the repository being +linted. In the case of ``ruff``, it might look like: + +.. code-block:: python + + import subprocess + from shutil import which + + def setup(root, **lintargs): + # This is a simple example. Please look at the actual source for better examples. + if not which("ruff"): + subprocess.call(["pip", "install", "ruff"]) + +The setup function will be called implicitly before running the linter. This +means it should return fast and not produce any output if there is no setup to +be performed. + +The setup functions can also be called explicitly by running ``mach lint +--setup``. This will only perform setup and not perform any linting. It is +mainly useful for other tools like ``mach bootstrap`` to call into. + + +Adding the linter to the CI +--------------------------- + +First, the job will have to be declared in Taskcluster. + +This should be done in the `mozlint Taskcluster configuration <https://searchfox.org/mozilla-central/source/taskcluster/ci/source-test/mozlint.yml>`_. +You will need to define a symbol, how it is executed and on what kind of change. + +For example, for ruff, the configuration is the following: + +.. code-block:: yaml + + py-ruff: + description: run ruff over the gecko codebase + treeherder: + symbol: py(ruff) + run: + mach: lint -l ruff -f treeherder -f json:/builds/worker/mozlint.json . + when: + files-changed: + - '**/*.py' + - '**/.ruff.toml' + +If the linter requires an external program, you will have to install it in the `setup script <https://searchfox.org/mozilla-central/source/taskcluster/docker/lint/system-setup.sh>`_ +and maybe install the necessary files in the `Docker configuration <https://searchfox.org/mozilla-central/source/taskcluster/docker/lint/Dockerfile>`_. + +.. note:: + + If the defect found by the linter is minor, make sure that it is logged as + a warning by setting `{"level": "warning"}` in the + :class:`~mozlint.result.Issue`. This means the defect will not cause a + backout if landed, but will still be surfaced by reviewbot at review time, + or when using `-W/--warnings` locally. diff --git a/docs/code-quality/lint/index.rst b/docs/code-quality/lint/index.rst new file mode 100644 index 0000000000..2b2cfdefbd --- /dev/null +++ b/docs/code-quality/lint/index.rst @@ -0,0 +1,33 @@ +Linting +======= + +Linters are used in mozilla-central to help enforce coding style and avoid bad practices. +They cover a wide variety of languages and checks. + +Getting Help +------------ + +If you need help or have questions, please don’t hesitate to contact us via Matrix +in the "Lint and Formatting" room +(`#lint:mozilla.org <https://chat.mozilla.org/#/room/#lint:mozilla.org>`_). + + +.. toctree:: + :caption: Getting Started + :maxdepth: 2 + + usage + +.. toctree:: + :caption: Linter Implementations + :maxdepth: 1 + :glob: + + linters/* + +.. toctree:: + :caption: Linter Specifics + :maxdepth: 1 + + mozlint + create diff --git a/docs/code-quality/lint/linters/android-format.rst b/docs/code-quality/lint/linters/android-format.rst new file mode 100644 index 0000000000..ed23ac64de --- /dev/null +++ b/docs/code-quality/lint/linters/android-format.rst @@ -0,0 +1,31 @@ +Spotless +======== + +`Spotless <https://github.com/diffplug/spotless>`__ is a pluggable formatter +for Gradle and Android. + +In our current configuration, Spotless includes the +`Google Java Format plug-in <https://github.com/google/google-java-format>`__ +which formats all our Java code using the Google Java coding style guidelines, +and `ktlint <https://ktlint.github.io/>`__ which formats all +our Kotlin code using the official Kotlin coding convention and Android Kotlin +Style Guide. + + +Run Locally +----------- + +The mozlint integration of spotless can be run using mach: + +.. parsed-literal:: + + $ mach lint --linter android-format + +Alternatively, omit the ``--linter android-format`` and run all configured linters, which will include +spotless. + + +Autofix +------- + +The spotless linter provides a ``--fix`` option. diff --git a/docs/code-quality/lint/linters/black.rst b/docs/code-quality/lint/linters/black.rst new file mode 100644 index 0000000000..60a06ce95b --- /dev/null +++ b/docs/code-quality/lint/linters/black.rst @@ -0,0 +1,36 @@ +Black +===== + +`Black <https://black.readthedocs.io/en/stable/>`__ is a opinionated python code formatter. + + +Run Locally +----------- + +The mozlint integration of black can be run using mach: + +.. parsed-literal:: + + $ mach lint --linter black <file paths> + +Alternatively, omit the ``--linter black`` and run all configured linters, which will include +black. + + +Configuration +------------- + +To enable black on new directory, add the path to the include +section in the :searchfox:`black.yml <tools/lint/black.yml>` file. + +Autofix +------- + +The black linter provides a ``--fix`` option. + + +Sources +------- + +* :searchfox:`Configuration (YAML) <tools/lint/black.yml>` +* :searchfox:`Source <tools/lint/python/black.py>` diff --git a/docs/code-quality/lint/linters/clang-format.rst b/docs/code-quality/lint/linters/clang-format.rst new file mode 100644 index 0000000000..a528af4358 --- /dev/null +++ b/docs/code-quality/lint/linters/clang-format.rst @@ -0,0 +1,35 @@ +clang-format +============ + +`clang-format <https://clang.llvm.org/docs/ClangFormat.html>`__ is a tool to reformat C/C++ to the right coding style. + +Run Locally +----------- + +The mozlint integration of clang-format can be run using mach: + +.. parsed-literal:: + + $ mach lint --linter clang-format <file paths> + + +Configuration +------------- + +To enable clang-format on new directory, add the path to the include +section in the :searchfox:`clang-format.yml <tools/lint/clang-format.yml>` file. + +While excludes: will work, this linter will read the ignore list from :searchfox:`.clang-format-ignore file <.clang-format-ignore>` +at the root directory. This because it is also used by the ./mach clang-format -p command. + +Autofix +------- + +clang-format can reformat the code with the option `--fix` (based on the upstream option `-i`). +To highlight the results, we are using the ``--dry-run`` option (from clang-format 10). + +Sources +------- + +* :searchfox:`Configuration (YAML) <tools/lint/clang-format.yml>` +* :searchfox:`Source <tools/lint/clang-format/__init__.py>` diff --git a/docs/code-quality/lint/linters/clippy.rst b/docs/code-quality/lint/linters/clippy.rst new file mode 100644 index 0000000000..40db532b88 --- /dev/null +++ b/docs/code-quality/lint/linters/clippy.rst @@ -0,0 +1,36 @@ +clippy +====== + +`clippy`_ is the tool for Rust static analysis. + +Run Locally +----------- + +The mozlint integration of clippy can be run using mach: + +.. parsed-literal:: + + $ mach lint --linter clippy <file paths> + +.. note:: + + clippy expects a path or a .rs file. It doesn't accept Cargo.toml + as it would break the mozlint workflow. + +Configuration +------------- + +To enable clippy on new directory, add the path to the include +section in the `clippy.yml <https://searchfox.org/mozilla-central/source/tools/lint/clippy.yml>`_ file. + +Autofix +------- + +This linter provides a ``--fix`` option. +Please note that this option does not fix all detected issues. + +Sources +------- + +* `Configuration (YAML) <https://searchfox.org/mozilla-central/source/tools/lint/clippy.yml>`_ +* `Source <https://searchfox.org/mozilla-central/source/tools/lint/clippy/__init__.py>`_ diff --git a/docs/code-quality/lint/linters/codespell.rst b/docs/code-quality/lint/linters/codespell.rst new file mode 100644 index 0000000000..9299a81b6e --- /dev/null +++ b/docs/code-quality/lint/linters/codespell.rst @@ -0,0 +1,36 @@ +Codespell +========= + +`codespell <https://github.com/codespell-project/codespell/>`__ is a popular tool to look for typical typos in the source code. + +It is enabled mostly for the documentation and English locale files. + +Run Locally +----------- + +The mozlint integration of codespell can be run using mach: + +.. parsed-literal:: + + $ mach lint --linter codespell <file paths> + + +Configuration +------------- + +To enable codespell on new directory, add the path to the include +section in the :searchfox:`codespell.yml <tools/lint/codespell.yml>` file. + +This job is configured as `tier 2 <https://wiki.mozilla.org/Sheriffing/Job_Visibility_Policy#Overview_of_the_Job_Visibility_Tiers>`_. + +Autofix +------- + +Codespell provides a ``--fix`` option. It is based on the ``-w`` option provided by upstream. + + +Sources +------- + +* :searchfox:`Configuration (YAML) <tools/lint/codespell.yml>` +* :searchfox:`Source <tools/lint/spell/__init__.py>` diff --git a/docs/code-quality/lint/linters/condprof-addons.rst b/docs/code-quality/lint/linters/condprof-addons.rst new file mode 100644 index 0000000000..c302eef5be --- /dev/null +++ b/docs/code-quality/lint/linters/condprof-addons.rst @@ -0,0 +1,85 @@ +CondProf Addons +=============== + +CondProf Addons is a linter for condprof customization JSON files (see :searchfox:`testing/condprofile/condprof/customization`), +it reports linting errors if: + +- any of the addons required by the customization files (e.g. see :searchfox:`testing/condprofile/condprof/customization/webext.json`) + is not found in the tar file fetched through the `firefox-addons` fetch task (see :searchfox:`taskcluster/ci/fetch/browsertime.yml`) +- or the expected `firefox-addons` fetch task has not been found + +Run Locally +----------- + +The mozlint integration of condprof-addons can be run using mach: + +.. parsed-literal:: + + $ mach lint --linter condprof-addons <file paths> + +Alternatively, if the ``--linter condprof-addons`` is omitted, the ``condprof-addons`` will still be selected automatically if +any of the files paths passed explicitly is detected to be part of the condprof customization directory. + +The ``condprof-addons`` will also be running automatically on ``mach lint --outgoing`` if there are customization files changes +detected in the outgoing patches. + +Run on Taskcluster +------------------ + +The condprof-addons job shows up as ``misc(condprof-addons)`` in the linting job. It should run automatically if changes are made +to condprof customization JSON files. + +Fix reported errors +------------------- + +XPI file is missing from the firefox-addons.tar archive +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +This linting errors is expected to be reported if the linter detected that a confprof customization file +requires an addon but the related xpi filename is not included in the firefox-addons.tar file fetched +through the `firefox-addons` fetch task (see :searchfox:`taskcluster/ci/fetch/browsertime.yml`). + +If the patch or phabricator revision is not meant to be landed, but only used as a temporary patch +pushed on try or only applied locally (e.g. to run the tp6/tp6m webextensions perftests with a given +third party extension installed to gather some metrics and/or GeckoProfiler data), then it can be +safely ignored. + +On the contrary, if the patch or phabricator revision is meant to be landed on mozilla-central, +the linting error have to be fixed before or along landing the change, either by: + +- removing the addition to the customization file if it wasn't intended to include that addon to all runs + of the tp6/tp6m webextensions perftests + +- updating the `firefox-addons` fetch task as defined in :searchfox:`taskcluster/ci/fetch/browsertime.yml` + by creating a pull request in the github repository where the asset is stored, and ask a review from + a peer of the `#webextensions-reviewer` and `#perftests-reviewers` review groups. + +firefox-addons taskcluster config 'add-prefix' attribute should be set to 'firefox-addons/' +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +If this linting error is hit, then the `firefox-addons` task defined in :searchfox:`taskcluster/ci/fetch/browsertime.yml` +is missing the `add-prefix` attribute or its value is not set to the expected 'firefox-addons/' subdir name. + +This is enforced as a linting rule because when the condprof utility is going to build a conditioned profile +for which some add-ons xpi files are expected to be sideloaded (see :searchfox:`testing/condprofile/condprof/customization.webext.json`), +to avoid re-downloading the same xpi from a remote urls every time the conditioned profile is built on the build infrastructure +(which for tp6/tp6m perftests will happen once per job being executed) condprof is going to look first if the expected xpi file +names are already available in `$MOZ_FETCHES_DIR/firefox-addons`. + +firefox-addons taskcluser fetch config section not found +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +This linting error is hit if the linter does not find the expected `firefox-addons` task defined in :searchfox:`taskcluster/ci/fetch/browsertime.yml` +or it is missing the expected `fetch` attribute. + +Configuration +------------- + +ConfProf Addons does not currently provide any configuration files. + +Sources +------- + +* :searchfox:`Configuration (YAML) <tools/lint/condprof-addons/yml>` +* :searchfox:`Source <tools/lint/condprof-addons/__init__.py>` +* :searchfox:`Test <tools/lint/test/test_condprof_addons.py>` diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla.rst new file mode 100644 index 0000000000..be6e158e86 --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla.rst @@ -0,0 +1,60 @@ +===================== +Mozilla ESLint Plugin +===================== + +This is the documentation of Mozilla ESLint PLugin. + +Environments +============ + +The plugin implements the following environments: + + +.. toctree:: + :maxdepth: 2 + + eslint-plugin-mozilla/environment + +Rules +===== + +The plugin implements the following rules: + +.. toctree:: + :maxdepth: 1 + :glob: + + eslint-plugin-mozilla/rules/* + +Tests +===== + +The tests for eslint-plugin-mozilla are run via `mochajs`_ on top of node. Most +of the tests use the `ESLint Rule Unit Test framework`_. + +.. _mochajs: https://mochajs.org/ +.. _ESLint Rule Unit Test Framework: http://eslint.org/docs/developer-guide/working-with-rules#rule-unit-tests + +Running Tests +------------- + +The tests for eslint-plugin-mozilla are run via `mochajs`_ on top of node. Most +of the tests use the `ESLint Rule Unit Test framework`_. + +The rules have some self tests, these can be run via: + +.. code-block:: shell + + $ cd tools/lint/eslint/eslint-plugin-mozilla + $ npm install + $ npm run test + +Disabling tests +--------------- + +In the unlikely event of needing to disable a test, currently the only way is +by commenting-out. Please file a bug if you have to do this. Bugs should be filed +in the *Testing* product under *Lint*. + +.. _mochajs: https://mochajs.org/ +.. _ESLint Rule Unit Test Framework: http://eslint.org/docs/developer-guide/working-with-rules#rule-unit-tests diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/environment.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/environment.rst new file mode 100644 index 0000000000..928fa3bc8b --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/environment.rst @@ -0,0 +1,91 @@ +Environment +=========== + +These environments are available by specifying a comment at the top of the file, +e.g. + +.. code-block:: js + + /* eslint-env mozilla/chrome-worker */ + +There are also built-in ESLint environments available as well. Find them here: http://eslint.org/docs/user-guide/configuring#specifying-environments + +browser-window +-------------- + +Defines the environment for scripts that are in the main browser.xhtml scope. + +chrome-script +------------- + +Defines the environment for scripts loaded by +``SpecialPowers.loadChromeScript``. + +chrome-worker +------------- + +Defines the environment for chrome workers. This differs from normal workers by +the fact that `ctypes` can be accessed as well. + +frame-script +------------ + +Defines the environment for scripts loaded by ``Services.mm.loadFrameScript``. + +jsm +--- + +Defines the environment for jsm files (javascript modules). + +privileged +---------- + +Defines the environment for privileged JS files. + +process-script +-------------- + +Defines the environment for scripts loaded by +``Services.ppmm.loadProcessScript``. + +remote-page +----------- + +Defines the environment for scripts loaded with ``<script src="...">`` in +``about:`` pages. + +simpletest +---------- + +Defines the environment for scripts that use the SimpleTest mochitest harness. + +sjs +--- + +Defines the environment for sjs files. + +special-powers-sandbox +---------------------- + +Defines the environment for scripts evaluated inside ``SpecialPowers`` sandbox +with the default options. + +testharness +----------- + +Defines the environment the globals that are injected from +:searchfox:`dom/imptests/testharness.js <dom/imptests/testharness.js>`. + +It is injected automatically into (x)html files which include: + +.. code-block:: html + + <script src="/resources/testharness.js"></script> + +It may need to be included manually in JavaScript files which are loaded into +the same scope. + +xpcshell +-------- + +Defines the environment for xpcshell test files. diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/avoid-Date-timing.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/avoid-Date-timing.rst new file mode 100644 index 0000000000..b01b568a28 --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/avoid-Date-timing.rst @@ -0,0 +1,30 @@ +avoid-Date-timing +================= + +Rejects grabbing the current time via Date.now() or new Date() for timing +purposes when the less problematic performance.now() can be used instead. + +The performance.now() function returns milliseconds since page load. To +convert that to milliseconds since the epoch, use: + +.. code-block:: js + + performance.timing.navigationStart + performance.now() + +Often timing relative to the page load is adequate and that conversion may not +be necessary. + +Examples of incorrect code for this rule: +----------------------------------------- + +.. code-block:: js + + Date.now() + +Examples of correct code for this rule: +--------------------------------------- + +.. code-block:: js + + new Date('2017-07-11'); + performance.now() diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/avoid-removeChild.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/avoid-removeChild.rst new file mode 100644 index 0000000000..15ece94d0d --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/avoid-removeChild.rst @@ -0,0 +1,20 @@ +avoid-removeChild +================= + +Rejects using ``element.parentNode.removeChild(element)`` when ``element.remove()`` +can be used instead. + +Examples of incorrect code for this rule: +----------------------------------------- + +.. code-block:: js + + elt.parentNode.removeChild(elt); + +Examples of correct code for this rule: +--------------------------------------- + +.. code-block:: js + + elt.remove(); + elt.parentNode.removeChild(elt2); diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/balanced-listeners.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/balanced-listeners.rst new file mode 100644 index 0000000000..f53c11e7aa --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/balanced-listeners.rst @@ -0,0 +1,20 @@ +balanced-listeners +================== + +Checks that for every occurrence of 'addEventListener' or 'on' there is an +occurrence of 'removeEventListener' or 'off' with the same event name. + +Examples of incorrect code for this rule: +----------------------------------------- + +.. code-block:: js + + elt.addEventListener('click', handler, false); + +Examples of correct code for this rule: +--------------------------------------- + +.. code-block:: js + + elt.addEventListener('event', handler); + elt.removeEventListener('event', handler); diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/balanced-observers.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/balanced-observers.rst new file mode 100644 index 0000000000..b169a520a3 --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/balanced-observers.rst @@ -0,0 +1,20 @@ +balanced-observers +================== + +Checks that for every occurrence of ``addObserver`` there is an +occurrence of ``removeObserver`` with the same topic. + +Examples of incorrect code for this rule: +----------------------------------------- + +.. code-block:: js + + Services.obs.addObserver(observer, 'observable'); + +Examples of correct code for this rule: +--------------------------------------- + +.. code-block:: js + + Services.obs.addObserver(observer, 'observable'); + Services.obs.removeObserver(observer, 'observable'); diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/consistent-if-bracing.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/consistent-if-bracing.rst new file mode 100644 index 0000000000..7bf6b796ef --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/consistent-if-bracing.rst @@ -0,0 +1,23 @@ +consistent-if-bracing +===================== + +Checks that if/elseif/else bodies are braced consistently, so either all bodies +are braced or unbraced. Doesn't enforce either of those styles though. + +Examples of incorrect code for this rule: +----------------------------------------- + +.. code-block:: js + + if (true) {1} else 0 + if (true) 1; else {0} + if (true) {1} else if (true) 2; else {0} + +Examples of correct code for this rule: +--------------------------------------- + +.. code-block:: js + + if (true) {1} else {0} + if (false) 1; else 0 + if (true) {1} else if (true) {2} else {0} diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/import-browser-window-globals.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/import-browser-window-globals.rst new file mode 100644 index 0000000000..35c09cc8fd --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/import-browser-window-globals.rst @@ -0,0 +1,8 @@ +import-browser-window-globals +============================= + +For scripts included in browser-window, this will automatically inject the +browser-window global scopes into the file. + +This is a rule rather than an environment, as it allowed us to automatically +select the files to include. diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/import-content-task-globals.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/import-content-task-globals.rst new file mode 100644 index 0000000000..f2550a1412 --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/import-content-task-globals.rst @@ -0,0 +1,14 @@ +import-content-task-globals +=========================== + +For files containing ContentTask.spawn calls, this will automatically declare +the frame script variables in the global scope. ContentTask is only available +to test files, so by default the configs only specify it for the mochitest based +configurations. + +This saves setting the file as a mozilla/frame-script environment. + +Note: due to the way ESLint works, it appears it is only easy to declare these +variables on a file global scope, rather than function global. This may mean that +they are incorrectly allowed, but given they are test files, this should be +detected during testing. diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/import-globals-from.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/import-globals-from.rst new file mode 100644 index 0000000000..c2956ba05a --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/import-globals-from.rst @@ -0,0 +1,18 @@ +import-globals-from +=================== + +Parses a file for globals defined in various unique Mozilla ways. + +When a ``/* import-globals-from <path> */`` comment is found in a file, then all +globals from the file at <path> will be imported in the current scope. This will +also operate recursively. + +This is useful for scripts that are loaded as <script> tag in a window and rely +on each other's globals. + +If <path> is a relative path, then it must be relative to the file being +checked by the rule. + +Note: ``import-globals-from`` does not support loading globals from ES modules. +These should be imported as variable definitions directly, or the file where +they are imported should be referenced. diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/import-globals.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/import-globals.rst new file mode 100644 index 0000000000..c420a1ca81 --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/import-globals.rst @@ -0,0 +1,5 @@ +import-globals +============== + +Checks ``ChromeUtils.defineLazyGetter`` etc and adds the name to the global +scope. diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/import-headjs-globals.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/import-headjs-globals.rst new file mode 100644 index 0000000000..df8ef81300 --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/import-headjs-globals.rst @@ -0,0 +1,26 @@ +import-headjs-globals +===================== + +Import globals from head.js and from any files that were imported by +head.js (as far as we can correctly resolve the path). + +This rule is included in the test configurations. + +The following file import patterns are supported: + +- ``Services.scriptloader.loadSubScript(path)`` +- ``loader.loadSubScript(path)`` +- ``loadSubScript(path)`` +- ``loadHelperScript(path)`` +- ``import-globals-from path`` + +If path does not exist because it is generated e.g. +``testdir + "/somefile.js"`` we do our best to resolve it. + +The following patterns are supported: + +- ``Cu.import("resource://devtools/client/shared/widgets/ViewHelpers.jsm");`` +- ``loader.lazyRequireGetter(this, "name2"`` +- ``loader.lazyServiceGetter(this, "name3"`` +- ``loader.lazyGetter(this, "toolboxStrings"`` +- ``ChromeUtils.defineLazyGetter(this, "clipboardHelper"`` diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/lazy-getter-object-name.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/lazy-getter-object-name.rst new file mode 100644 index 0000000000..7d58fd631b --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/lazy-getter-object-name.rst @@ -0,0 +1,25 @@ +lazy-getter-object-name +============================= + +Enforce the standard object variable name ``lazy`` for +``ChromeUtils.defineESModuleGetters`` + +Examples of incorrect code for this rule: +----------------------------------------- + +.. code-block:: js + + const obj = {}; + ChromeUtils.defineESModuleGetters(obj, { + AppConstants: "resource://gre/modules/AppConstants.sys.mjs", + }); + +Examples of correct code for this rule: +--------------------------------------- + +.. code-block:: js + + const lazy = {}; + ChromeUtils.defineESModuleGetters(lazy, { + AppConstants: "resource://gre/modules/AppConstants.sys.mjs", + }); diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/mark-exported-symbols-as-used.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/mark-exported-symbols-as-used.rst new file mode 100644 index 0000000000..92e315a249 --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/mark-exported-symbols-as-used.rst @@ -0,0 +1,23 @@ +mark-exported-symbols-as-used +============================= + +Marks variables listed in ``EXPORTED_SYMBOLS`` as used so that ``no-unused-vars`` +does not complain about them. + +This rule also checks that ``EXPORTED_SYMBOLS`` is not defined using ``let`` as +``let`` isn't allowed as the lexical scope may die after the script executes. + +Examples of incorrect code for this rule: +----------------------------------------- + +.. code-block:: js + + let EXPORTED_SYMBOLS = ["foo"]; + +Examples of correct code for this rule: +--------------------------------------- + +.. code-block:: js + + var EXPORTED_SYMBOLS = ["foo"]; + const EXPORTED_SYMBOLS = ["foo"]; diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/mark-test-function-used.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/mark-test-function-used.rst new file mode 100644 index 0000000000..a518d7415b --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/mark-test-function-used.rst @@ -0,0 +1,8 @@ +mark-test-function-used +======================= + +Simply marks ``test`` (the test method) or ``run_test`` as used when in mochitests +or xpcshell tests respectively. This avoids ESLint telling us that the function +is never called. + +This rule is included in the test configurations. diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/no-aArgs.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/no-aArgs.rst new file mode 100644 index 0000000000..7e398bcbbe --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/no-aArgs.rst @@ -0,0 +1,22 @@ +no-aArgs +======== + +Checks that function argument names don't start with lowercase 'a' followed by +a capital letter. This is to prevent the use of Hungarian notation whereby the +first letter is a prefix that indicates the type or intended use of a variable. + +Examples of incorrect code for this rule: +----------------------------------------- + +.. code-block:: js + + function(aFoo, aBar) {} + (aFoo, aBar) => {} + +Examples of correct code for this rule: +--------------------------------------- + +.. code-block:: js + + function(foo, bar) {} + (foo, bar) => {}) diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/no-addtask-setup.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/no-addtask-setup.rst new file mode 100644 index 0000000000..f26a869371 --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/no-addtask-setup.rst @@ -0,0 +1,27 @@ +no-addtask-setup +================ + +Reject using ``add_task(async function setup() { ... })`` in favour of +``add_setup(async function() { ... })``. + +Using semantically separate setup functions makes ``.only`` work correctly +and will allow for future improvements to setup/cleanup abstractions. + +This option can be autofixed (``--fix``). + +Examples of incorrect code for this rule: +----------------------------------------- + +.. code-block:: js + + add_task(async function setup() { ... }); + add_task(function setup() { ... }); + add_task(function init() { ... }); + +Examples of correct code for this rule: +--------------------------------------- + +.. code-block:: js + + add_setup(async function() { ... }); + add_setup(function() { ... }); diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/no-arbitrary-setTimeout.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/no-arbitrary-setTimeout.rst new file mode 100644 index 0000000000..a7d62e74ba --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/no-arbitrary-setTimeout.rst @@ -0,0 +1,23 @@ +no-arbitrary-setTimeout +======================= + +Disallows setTimeout with non-zero values in tests. Using arbitrary times for +setTimeout may cause intermittent failures in tests. A value of zero is allowed +as this is letting the event stack unwind, however also consider the use +of ``TestUtils.waitForTick``. + +Examples of incorrect code for this rule: +----------------------------------------- + +.. code-block:: js + + function(aFoo, aBar) {} + (aFoo, aBar) => {} + +Examples of correct code for this rule: +--------------------------------------- + +.. code-block:: js + + function(foo, bar) {} + (foo, bar) => {}) diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/no-browser-refs-in-toolkit.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/no-browser-refs-in-toolkit.rst new file mode 100644 index 0000000000..9f2501ba23 --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/no-browser-refs-in-toolkit.rst @@ -0,0 +1,24 @@ +no-browser-refs-in-toolkit +========================== + +Rejects for using any code from ``browser/`` (Desktop Firefox) inside +``toolkit``, as ``toolkit`` is for code shared with other Gecko consumers +like Firefox on Android, Thunderbird, etc. + +Examples of incorrect code for this rule: +----------------------------------------- + +.. code-block:: js + + "chrome://browser/content/browser.xhtml" + "resource:///modules/BrowserWindowTracker.sys.mjs" + "browser/browser.ftl" + +Examples of correct code for this rule: +--------------------------------------- + +.. code-block:: js + + "chrome://global/content/aboutAbout.html" + "resource://gre/modules/AppConstants.sys.mjs" + "toolkit/global/aboutFoo.ftl" diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/no-compare-against-boolean-literals.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/no-compare-against-boolean-literals.rst new file mode 100644 index 0000000000..b7785f2fc2 --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/no-compare-against-boolean-literals.rst @@ -0,0 +1,23 @@ +no-compare-against-boolean-literals +=================================== + +Checks that boolean expressions do not compare against literal values +of ``true`` or ``false``. This is to prevent overly verbose code such as +``if (isEnabled == true)`` when ``if (isEnabled)`` would suffice. + +Examples of incorrect code for this rule: +----------------------------------------- + +.. code-block:: js + + if (foo == true) {} + if (foo != false) {} + if (false == foo) {} + +Examples of correct code for this rule: +--------------------------------------- + +.. code-block:: js + + if (!foo) {} + if (!!foo) {} diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/no-comparison-or-assignment-inside-ok.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/no-comparison-or-assignment-inside-ok.rst new file mode 100644 index 0000000000..33776433db --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/no-comparison-or-assignment-inside-ok.rst @@ -0,0 +1,69 @@ +no-comparison-or-assignment-inside-ok +===================================== + +This rule prevents two misuses of the ``ok()`` test framework function. + +Assignments +----------- + +The first is one where people accidentally put assignments into ``ok``, like so: + +.. code-block:: js + + ok(foo = bar, "Foo should be equal to bar"); + +This is similar to the builtin eslint rule `no-cond-assign`_. + +There is no autofix as the linter cannot tell if the assignment was intentional +and should be moved out of the ``ok()`` call, or if it was intended to be some +kind of binary comparison expression (and if so, with what operator). + +.. _no-cond-assign: https://eslint.org/docs/latest/rules/no-cond-assign + +Comparisons +----------- + +The second is a stylistic/legibility/debuggability preference, where the linter +encourages use of the dedicated ``Assert`` framework comparison functions. For +more details on ``Assert``, see :ref:`assert-module`. + +This rule is autofixed, and will correct incorrect examples such as the +following: + +Examples of incorrect code for this rule: +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +.. code-block:: js + + ok(foo == bar); + ok(foo < bar); + ok(foo !== bar); + +to something like: + +Examples of correct code for this rule: +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +.. code-block:: js + + Assert.equal(foo, bar); + Assert.less(foo, bar); + Assert.notStrictEqual(foo, bar); + + +Rationale +^^^^^^^^^ + +There are a few reasons the more verbose form is preferable: + +- On failure, the framework will log both values rather than just one, making + it much easier to debug failing tests without having to manually add + logging, push to try, and then potentially retrigger to reproduce intermittents + etc. +- The :ref:`assert-module` is standardized and more widely understood than the old + mocha/mochikit forms. +- It is harder to make typos like the assignment case above, and accidentally + end up with tests that assert non-sensical things. +- Subjectively, when an additional message is long enough to cause ``prettier`` + to split the statement across multiple lines, this makes it easier to + see what the two operands of the comparison are. diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/no-cu-reportError.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/no-cu-reportError.rst new file mode 100644 index 0000000000..9f5a0def27 --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/no-cu-reportError.rst @@ -0,0 +1,23 @@ +no-cu-reportError +================= + +Disallows Cu.reportError. This has been deprecated and should be replaced by +console.error. + +Examples of incorrect code for this rule: +----------------------------------------- + +.. code-block:: js + + Cu.reportError("message"); + Cu.reportError("message", stack); + +Examples of correct code for this rule: +--------------------------------------- + +.. code-block:: js + + console.error("message"); + let error = new Error("message"); + error.stack = stack; + console.error(error); diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/no-define-cc-etc.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/no-define-cc-etc.rst new file mode 100644 index 0000000000..4421f4dd54 --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/no-define-cc-etc.rst @@ -0,0 +1,23 @@ +no-define-cc-etc +================ + +Disallows old-style definitions for ``Cc``/``Ci``/``Cu``/``Cr``. These are now +defined globally for all chrome contexts. + +Examples of incorrect code for this rule: +----------------------------------------- + +.. code-block:: js + + var Cc = Components.classes; + var Ci = Components.interfaces; + var {Ci: interfaces, Cc: classes, Cu: utils} = Components; + var Cr = Components.results; + + +Examples of correct code for this rule: +--------------------------------------- + +.. code-block:: js + + const CC = Components.Constructor; diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/no-redeclare-with-import-autofix.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/no-redeclare-with-import-autofix.rst new file mode 100644 index 0000000000..d7d4edab50 --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/no-redeclare-with-import-autofix.rst @@ -0,0 +1,21 @@ +no-redeclare-with-import-autofix +================================ + +This is the +`builtin eslint rule no-redeclare <https://eslint.org/docs/latest/rules/no-redeclare>`_, +but with an additional fixer that can automatically remove superfluous +(duplicate) imports. + +For redeclarations that are not imports, there is no automatic fix, as +the author will likely have to rename variables so that they do not +redeclare existing variables or conflict with the name of a builtin +property or global variable. + +Typical duplicate imports happen when a `head.js` file, imports +a module and a test then subsequently also attempts to import the same +module. + +In browser mochitests, an additional typical scenario is importing +modules that are already imported in the main browser window. Because +these tests run in a scope that inherits from the main browser window +one, there is no need to re-import such modules. diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/no-throw-cr-literal.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/no-throw-cr-literal.rst new file mode 100644 index 0000000000..0f9222de30 --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/no-throw-cr-literal.rst @@ -0,0 +1,38 @@ +no-throw-cr-literal +=================== + +This is similar to the ESLint built-in rule no-throw-literal. It disallows +throwing Components.results code directly. + +Throwing bare literals is inferior to throwing Exception objects, which provide +stack information. Cr.ERRORs should be be passed as the second argument to +``Components.Exception()`` to create an Exception object with stack info, and +the correct result property corresponding to the NS_ERROR that other code +expects. +Using a regular ``new Error()`` to wrap just turns it into a string and doesn't +set the result property, so the errors can't be recognised. + +This option can be autofixed (``--fix``). + +.. code-block:: js + + performance.timing.navigationStart + performance.now() + +Often timing relative to the page load is adequate and that conversion may not +be necessary. + +Examples of incorrect code for this rule: +----------------------------------------- + +.. code-block:: js + + throw Cr.NS_ERROR_UNEXPECTED; + throw Components.results.NS_ERROR_ABORT; + throw new Error(Cr.NS_ERROR_NO_INTERFACE); + +Examples of correct code for this rule: +--------------------------------------- + +.. code-block:: js + + throw Components.Exception("Not implemented", Cr.NS_ERROR_NOT_IMPLEMENTED); diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/no-useless-parameters.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/no-useless-parameters.rst new file mode 100644 index 0000000000..485caf6522 --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/no-useless-parameters.rst @@ -0,0 +1,26 @@ +no-useless-parameters +===================== + +Reject common XPCOM methods called with useless optional parameters (eg. +``Services.io.newURI(url, null, null)``, or non-existent parameters (eg. +``Services.obs.removeObserver(name, observer, false)``). + +This option can be autofixed (``--fix``). + +Examples of incorrect code for this rule: +----------------------------------------- + +.. code-block:: js + + elt.addEventListener('click', handler, false); + Services.io.newURI('http://example.com', null, null); + Services.obs.notifyObservers(obj, 'topic', null); + +Examples of correct code for this rule: +--------------------------------------- + +.. code-block:: js + + elt.addEventListener('click', handler); + Services.io.newURI('http://example.com'); + Services.obs.notifyObservers(obj, 'topic'); diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/no-useless-removeEventListener.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/no-useless-removeEventListener.rst new file mode 100644 index 0000000000..ce314ab58d --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/no-useless-removeEventListener.rst @@ -0,0 +1,20 @@ +no-useless-removeEventListener +============================== + +Reject calls to removeEventListener where ``{once: true}`` could be used instead. + +Examples of incorrect code for this rule: +----------------------------------------- + +.. code-block:: js + + elt.addEventListener('click', function listener() { + elt.removeEventListener('click', listener); + }); + +Examples of correct code for this rule: +--------------------------------------- + +.. code-block:: js + + elt.addEventListener('click', handler, {once: true}); diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/no-useless-run-test.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/no-useless-run-test.rst new file mode 100644 index 0000000000..a079405696 --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/no-useless-run-test.rst @@ -0,0 +1,6 @@ +no-useless-run-test +=================== + +Designed for xpcshell-tests. Rejects definitions of ``run_test()`` where the +function only contains a single call to ``run_next_test()``. xpcshell's head.js +already defines a utility function so there is no need for duplication. diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/prefer-boolean-length-check.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/prefer-boolean-length-check.rst new file mode 100644 index 0000000000..cd6ee4e544 --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/prefer-boolean-length-check.rst @@ -0,0 +1,24 @@ +prefer-boolean-length-check +=========================== + +Prefers using a boolean length check rather than comparing against zero. + +Examples of incorrect code for this rule: +----------------------------------------- + +.. code-block:: js + + if (foo.length == 0) {} + if (foo.length > 0) {} + if (foo && foo.length == 0) {} + function bar() { return foo.length > 0 } + +Examples of correct code for this rule: +--------------------------------------- + +.. code-block:: js + + if (foo.length && foo.length) {} + if (!foo.length) {} + var a = foo.length > 0 + function bar() { return !!foo.length } diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/prefer-formatValues.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/prefer-formatValues.rst new file mode 100644 index 0000000000..88eedee792 --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/prefer-formatValues.rst @@ -0,0 +1,23 @@ +prefer-formatValues +=================== + +Rejects multiple calls to document.l10n.formatValue in the same code block, to +reduce localization overheads. + +Examples of incorrect code for this rule: +----------------------------------------- + +.. code-block:: js + + { + document.l10n.formatValue('foobar'); + document.l10n.formatValue('foobaz'); + } + +Examples of correct code for this rule: +--------------------------------------- + +.. code-block:: js + + document.l10n.formatValue('foobar'); + document.l10n.formatValues(['foobar', 'foobaz']); diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/reject-addtask-only.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/reject-addtask-only.rst new file mode 100644 index 0000000000..e540b24416 --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/reject-addtask-only.rst @@ -0,0 +1,6 @@ +reject-addtask-only +=================== + +Designed for JavaScript tests using the add_task pattern. Rejects chaining +.only() to an add_task() call, which is useful for local testing to run a +single task in isolation but is easy to land into the tree by accident. diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/reject-chromeutils-import-params.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/reject-chromeutils-import-params.rst new file mode 100644 index 0000000000..4710878f8d --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/reject-chromeutils-import-params.rst @@ -0,0 +1,22 @@ +reject-chromeutils-import-params +================================ + +``ChromeUtils.import`` used to be able to be called with two arguments, however +the second argument is no longer supported. Exports from modules should now be +explicit, and the imported symbols being accessed from the returned object. + +Examples of incorrect code for this rule: +----------------------------------------- + +.. code-block:: js + + ChromeUtils.import("resource://gre/modules/AppConstants.jsm", this); + ChromeUtils.import("resource://gre/modules/AppConstants.jsm", null); + ChromeUtils.import("resource://gre/modules/AppConstants.jsm", {}); + +Examples of correct code for this rule: +--------------------------------------- + +.. code-block:: js + + const { AppConstants } = ChromeUtils.import("resource://gre/modules/AppConstants.jsm"); diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/reject-chromeutils-import.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/reject-chromeutils-import.rst new file mode 100644 index 0000000000..5c310fb607 --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/reject-chromeutils-import.rst @@ -0,0 +1,27 @@ +reject-chromeutils-import +================================ + +While ``ChromeUtils.import`` supports the fallback to ESMified module, +in-tree files should use ``ChromeUtils.importESModule`` for ESMified modules. + +Examples of incorrect code for this rule: +----------------------------------------- + +.. code-block:: js + + ChromeUtils.import("resource://gre/modules/AppConstants.jsm"); + + ChromeUtils.defineModuleGetter( + obj, "AppConstants", "resource://gre/modules/AppConstants.jsm"); + XPCOMUtils.defineLazyModuleGetters( + obj, { AppConstants: "resource://gre/modules/AppConstants.jsm" }); + +Examples of correct code for this rule: +--------------------------------------- + +.. code-block:: js + + ChromeUtils.importESModule("resource://gre/modules/AppConstants.sys.mjs"); + + ChromeUtils.defineESModuleGetters( + obj, { AppConstants: "resource://gre/modules/AppConstants.sys.mjs" }); diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/reject-eager-module-in-lazy-getter.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/reject-eager-module-in-lazy-getter.rst new file mode 100644 index 0000000000..4dc29f4431 --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/reject-eager-module-in-lazy-getter.rst @@ -0,0 +1,30 @@ +reject-eager-module-in-lazy-getter +================================== + +Rejects defining a lazy getter for module that's known to be loaded early in the +startup process and it is not necessary to lazy load it. + +Examples of incorrect code for this rule: +----------------------------------------- + +.. code-block:: js + + ChromeUtils.defineESModuleGetters(lazy, { + AppConstants: "resource://gre/modules/AppConstants.sys.mjs", + }); + XPCOMUtils.defineLazyModuleGetters(lazy, { + XPCOMUtils: "resource://gre/modules/XPCOMUtils.jsm", + }); + +Examples of correct code for this rule: +--------------------------------------- + +.. code-block:: js + + import { AppConstants } from "resource://gre/modules/AppConstants.sys.mjs"; + const { XPCOMUtils } = ChromeUtils.import( + "resource://gre/modules/XPCOMUtils.jsm" + ); + const { AppConstants } = ChromeUtils.import( + "resource://gre/modules/AppConstants.jsm" + ); diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/reject-global-this.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/reject-global-this.rst new file mode 100644 index 0000000000..b3d94321f5 --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/reject-global-this.rst @@ -0,0 +1,29 @@ +reject-global-this +====================== + +Rejects global ``this`` usage in JSM files. The global ``this`` is not +available in ESM, and this is a preparation for the migration. + +Examples of incorrect code for this rule: +----------------------------------------- + +.. code-block:: js + + this.EXPORTED_SYMBOLS = ["foo"]; + + XPCOMUtils.defineLazyModuleGetters(this, { + AddonManager: "resource://gre/modules/AddonManager.jsm", + }); + + +Examples of correct code for this rule: +--------------------------------------- + +.. code-block:: js + + const EXPORTED_SYMBOLS = ["foo"]; + + const lazy = {}; + XPCOMUtils.defineLazyModuleGetters(lazy, { + AddonManager: "resource://gre/modules/AddonManager.jsm", + }); diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/reject-globalThis-modification.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/reject-globalThis-modification.rst new file mode 100644 index 0000000000..dd4fc4b2af --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/reject-globalThis-modification.rst @@ -0,0 +1,19 @@ +reject-globalThis-modification +============================== + +Reject any modification to ``globalThis`` inside the system modules. + +``globalThis`` is the shared global inside the system modules, and modification +on it is visible from all modules, and it shouldn't be done unless it's really +necessary. + +Examples of incorrect code for this rule: +----------------------------------------- + +.. code-block:: js + + globalThis.foo = 10; + Object.defineProperty(globalThis, "bar", { value: 20}); + ChromeUtils.defineESModuleGetters(globalThis, { + AppConstants: "resource://gre/modules/AppConstants.sys.mjs", + }); diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/reject-import-system-module-from-non-system.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/reject-import-system-module-from-non-system.rst new file mode 100644 index 0000000000..d168676745 --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/reject-import-system-module-from-non-system.rst @@ -0,0 +1,36 @@ +reject-import-system-module-from-non-system +=========================================== + +Rejects static import declaration for system modules (``.sys.mjs``) from non-system +modules. + +Using static import for a system module into a non-system module would create a separate instance of the imported object(s) that is not shared with the other system modules and would break the per-process singleton expectation. + +The reason for this is that inside system modules, a static import will load the module into the shared global. Inside non-system modules, the static import will load into a different global (e.g. window). This will cause the module to be loaded into different scopes, and hence create separate instances. The fix is to use ``ChromeUtils.importESModule`` which will import the object via the system module shared global scope. + + +Examples of incorrect code for this rule: +----------------------------------------- + +Inside a non-system module: + +.. code-block:: js + + import { AppConstants } from "resource://gre/modules/AppConstants.sys.mjs"; + +Examples of correct code for this rule: +--------------------------------------- + +Inside a non-system module: + +.. code-block:: js + + const { AppConstants } = ChromeUtils.importESModule( + "resource://gre/modules/AppConstants.sys.mjs" + ); + +Inside a system module: + +.. code-block:: js + + import { AppConstants } from "resource://gre/modules/AppConstants.sys.mjs"; diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/reject-importGlobalProperties.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/reject-importGlobalProperties.rst new file mode 100644 index 0000000000..60e2a3887b --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/reject-importGlobalProperties.rst @@ -0,0 +1,48 @@ +reject-importGlobalProperties +============================= + +Rejects calls to ``Cu.importGlobalProperties`` or +``XPCOMUtils.defineLazyGlobalGetters``. + +In system modules all the required properties should already be available. In +non-module code or non-system modules, webidl defined interfaces should already +be available and hence do not need importing. + +For sjs test files, if the relevant global is not already available, then consider +extending the `list of globals available from the httpd server <https://searchfox.org/mozilla-central/rev/e9b338c2d597067f99e96d5f20769f41f312fa8f/netwerk/test/httpserver/httpd.sys.mjs#2875-2889>`_. + +Options +------- + +* "everything": Disallows using the import/getters completely. +* "allownonwebidl": Disallows using the import functions for webidl symbols. Allows + other symbols. + +everything +---------- + +Incorrect code for this option: + +.. code-block:: js + + Cu.importGlobalProperties(['TextEncoder']); + XPCOMUtils.defineLazyGlobalGetters(this, ['TextEncoder']); + +allownonwebidl +-------------- + +Incorrect code for this option: + +.. code-block:: js + + // AnimationEffect is a webidl property. + Cu.importGlobalProperties(['AnimationEffect']); + XPCOMUtils.defineLazyGlobalGetters(this, ['AnimationEffect']); + +Correct code for this option: + +.. code-block:: js + + // TextEncoder is not defined by webidl. + Cu.importGlobalProperties(['TextEncoder']); + XPCOMUtils.defineLazyGlobalGetters(this, ['TextEncoder']); diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/reject-lazy-imports-into-globals.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/reject-lazy-imports-into-globals.rst new file mode 100644 index 0000000000..7fc2beb8ae --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/reject-lazy-imports-into-globals.rst @@ -0,0 +1,33 @@ +reject-lazy-imports-into-globals +================================ + +Rejects importing lazy items into ``window`` or ``globalThis`` when in a +non-system module scope. + +Importing into the ``window`` scope (or ``globalThis``) will share the imported +global with everything else in the same window. In modules, this is generally +unnecessary and undesirable because each module imports what it requires. +Additionally, sharing items via the global scope makes it more difficult for +linters to determine the available globals. + +Instead, the globals should either be imported directly, or into a lazy object. +If there is a good reason for sharing the globals via the ``window`` scope, then +this rule may be disabled as long as a comment is added explaining the reasons. + +Examples of incorrect code for this rule: +----------------------------------------- + +.. code-block:: js + + ChromeUtils.defineESModuleGetters(globalThis, { foo: "foo.sys.mjs" }); + ChromeUtils.defineESModuleGetters(window, { "foo": "foo.sys.mjs" }); + ChromeUtils.defineLazyGetter(globalThis, "foo", () => {}); + ChromeUtils.defineLazyGetter(window, "foo", () => {}); + +Examples of correct code for this rule: +--------------------------------------- + +.. code-block:: js + + const lazy = {}; + ChromeUtils.defineLazyGetter(lazy, "foo", () => {}); diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/reject-mixing-eager-and-lazy.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/reject-mixing-eager-and-lazy.rst new file mode 100644 index 0000000000..51524f4e14 --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/reject-mixing-eager-and-lazy.rst @@ -0,0 +1,22 @@ +reject-mixing-eager-and-lazy +================================== + +Rejects defining a lazy getter for a module that's eagerly imported at +top-level script unconditionally. + +Examples of incorrect code for this rule: +----------------------------------------- + +.. code-block:: js + + const { SomeProp } = ChromeUtils.import("resource://gre/modules/Foo.jsm"); + ChromeUtils.defineLazyModuleGetters(lazy, { + OtherProp: "resource://gre/modules/Foo.jsm", + }); + +Examples of correct code for this rule: +--------------------------------------- + +.. code-block:: js + + const { SomeProp, OtherProp } = ChromeUtils.import("resource://gre/modules/Foo.jsm"); diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/reject-multiple-getters-calls.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/reject-multiple-getters-calls.rst new file mode 100644 index 0000000000..7ea048402b --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/reject-multiple-getters-calls.rst @@ -0,0 +1,27 @@ +reject-multiple-getters-calls +============================= + +Rejects multiple calls on ``ChromeUtils.defineESModuleGetters`` for the same +target in the same context. + +Examples of incorrect code for this rule: +----------------------------------------- + +.. code-block:: js + + ChromeUtils.defineESModuleGetters(lazy, { + AppConstants: "resource://gre/modules/AppConstants.sys.mjs", + }); + ChromeUtils.defineESModuleGetters(lazy, { + PlacesUtils: "resource://gre/modules/PlacesUtils.sys.mjs", + }); + +Examples of correct code for this rule: +--------------------------------------- + +.. code-block:: js + + ChromeUtils.defineESModuleGetters(lazy, { + AppConstants: "resource://gre/modules/AppConstants.sys.mjs", + PlacesUtils: "resource://gre/modules/PlacesUtils.sys.mjs", + }); diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/reject-relative-requires.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/reject-relative-requires.rst new file mode 100644 index 0000000000..4387041b26 --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/reject-relative-requires.rst @@ -0,0 +1,22 @@ +reject-relative-requires +======================== + +Rejects calls to require which use relative directories. + +Examples of incorrect code for this rule: +----------------------------------------- + +.. code-block:: js + + require("./relative/path") + require("../parent/folder/path") + loader.lazyRequireGetter(this, "path", "../parent/folder/path", true) + +Examples of correct code for this rule: +--------------------------------------- + +.. code-block:: js + + require("devtools/absolute/path") + require("resource://gre/modules/SomeModule.jsm") + loader.lazyRequireGetter(this, "path", "devtools/absolute/path", true) diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/reject-requires-await.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/reject-requires-await.rst new file mode 100644 index 0000000000..2a8618939f --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/reject-requires-await.rst @@ -0,0 +1,20 @@ +reject-requires-await +===================== + +`Assert.rejects` must be preceded by an `await`, otherwise the assertion +may not be completed before the test finishes, might not be caught +and might cause intermittent issues in other tests. + +Examples of incorrect code for this rule: +----------------------------------------- + +.. code-block:: js + + Assert.rejects(myfunc(), /startup failed/, "Should reject"); + +Examples of correct code for this rule: +--------------------------------------- + +.. code-block:: js + + await Assert.rejects(myfunc(), /startup failed/, "Should reject"); diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/reject-scriptableunicodeconverter.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/reject-scriptableunicodeconverter.rst new file mode 100644 index 0000000000..8f6ae39060 --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/reject-scriptableunicodeconverter.rst @@ -0,0 +1,13 @@ +reject-scriptableunicodeconverter +================================================ + +Rejects calls into ``Ci.nsIScriptableUnicodeConverter``. This is configured as a warning. +You should use |TextEncoder|_ or |TextDecoder|_ for new code. +If modifying old code, please consider swapping it in if possible; if this is tricky please ensure +a bug is on file. + +.. |TextEncoder| replace:: ``TextEncoder`` +.. _TextEncoder: https://searchfox.org/mozilla-central/source/dom/webidl/TextEncoder.webidl + +.. |TextDecoder| replace:: ``TextDecoder`` +.. _TextDecoder: https://searchfox.org/mozilla-central/source/dom/webidl/TextDecoder.webidl diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/reject-some-requires.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/reject-some-requires.rst new file mode 100644 index 0000000000..476dcbcb94 --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/reject-some-requires.rst @@ -0,0 +1,6 @@ +reject-some-requires +==================== + +This takes an option, a regular expression. Invocations of +``require`` with a string literal argument are matched against this +regexp; and if it matches, the ``require`` use is flagged. diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/reject-top-level-await.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/reject-top-level-await.rst new file mode 100644 index 0000000000..38be0b2d22 --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/reject-top-level-await.rst @@ -0,0 +1,26 @@ +reject-top-level-await +====================== + +Rejects ``await`` at the top-level of code in modules. Top-level ``await`` is +not currently support in Gecko's component modules, so this is rejected. + +Examples of incorrect code for this rule: +----------------------------------------- + +.. code-block:: js + + await foo; + + if (expr) { + await foo; + } + + for await (let x of [1, 2, 3]) { } + +Examples of correct code for this rule: +--------------------------------------- + +.. code-block:: js + + async function() { await foo; } + async function() { for await (let x of [1, 2, 3]) { } } diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/use-cc-etc.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/use-cc-etc.rst new file mode 100644 index 0000000000..902b4a630c --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/use-cc-etc.rst @@ -0,0 +1,26 @@ +use-cc-etc +====================== + +This requires using ``Cc`` rather than ``Components.classes``, and the same for +``Components.interfaces``, ``Components.results`` and ``Components.utils``. +This has a slight performance advantage by avoiding the use of the dot. + +Examples of incorrect code for this rule: +----------------------------------------- + +.. code-block:: js + + let foo = Components.classes['bar']; + let bar = Components.interfaces.bar; + Components.results.NS_ERROR_ILLEGAL_INPUT; + Components.utils.reportError('fake'); + +Examples of correct code for this rule: +--------------------------------------- + +.. code-block:: js + + let foo = Cc['bar']; + let bar = Ci.bar; + Cr.NS_ERROR_ILLEGAL_INPUT; + Cu.reportError('fake'); diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/use-chromeutils-definelazygetter.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/use-chromeutils-definelazygetter.rst new file mode 100644 index 0000000000..61d13f4e63 --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/use-chromeutils-definelazygetter.rst @@ -0,0 +1,22 @@ +use-chromeutils-definelazygetter +================================ + +Require use of ``ChromeUtils.defineLazyGetter`` rather than ``XPCOMUtils.defineLazyGetter``. + +Examples of incorrect code for this rule: +----------------------------------------- + +.. code-block:: js + + XPCOMUtils.defineLazyGetter(lazy, "textEncoder", function () { + return new TextEncoder(); + }); + +Examples of correct code for this rule: +--------------------------------------- + +.. code-block:: js + + ChromeUtils.defineLazyGetter(lazy, "textEncoder", function () { + return new TextEncoder(); + }); diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/use-chromeutils-generateqi.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/use-chromeutils-generateqi.rst new file mode 100644 index 0000000000..3da22e139a --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/use-chromeutils-generateqi.rst @@ -0,0 +1,33 @@ +use-chromeutils-generateqi +========================== + +Reject use of ``XPCOMUtils.generateQI`` and JS-implemented QueryInterface +methods in favor of ``ChromeUtils``. + +Examples of incorrect code for this rule: +----------------------------------------- + +.. code-block:: js + + X.prototype.QueryInterface = XPCOMUtils.generateQI(["nsIMeh"]); + X.prototype = { QueryInterface: XPCOMUtils.generateQI(["nsIMeh"]) }; + X.prototype = { QueryInterface: function QueryInterface(iid) { + if ( + iid.equals(Ci.nsISupports) || + iid.equals(Ci.nsIMeh) || + iid.equals(nsIFlug) || + iid.equals(Ci.amIFoo) + ) { + return this; + } + throw Components.Exception("", Cr.NS_ERROR_NO_INTERFACE); + } }; + + +Examples of correct code for this rule: +--------------------------------------- + +.. code-block:: js + + X.prototype.QueryInterface = ChromeUtils.generateQI(["nsIMeh"]); + X.prototype = { QueryInterface: ChromeUtils.generateQI(["nsIMeh"]) } diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/use-chromeutils-import.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/use-chromeutils-import.rst new file mode 100644 index 0000000000..3072861492 --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/use-chromeutils-import.rst @@ -0,0 +1,19 @@ +use-chromeutils-import +====================== + +Require use of ``ChromeUtils.import`` rather than ``Components.utils.import``. + +Examples of incorrect code for this rule: +----------------------------------------- + +.. code-block:: js + + Components.utils.import("resource://gre/modules/AppConstants.jsm", this); + +Examples of correct code for this rule: +--------------------------------------- + +.. code-block:: js + + ChromeUtils.import("resource://gre/modules/AppConstants.jsm", this); + ChromeUtils.defineModuleGetter(this, "AppConstants", "resource://gre/modules/AppConstants.jsm"); diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/use-console-createInstance.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/use-console-createInstance.rst new file mode 100644 index 0000000000..b505cfd8fb --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/use-console-createInstance.rst @@ -0,0 +1,20 @@ +use-console-createInstance +========================== + +Rejects usage of Console.sys.mjs or Log.sys.mjs, preferring use of +``console.createInstance`` instead. See `Javascript Logging </toolkit/javascript-logging.html>`__ for more information. + +Examples of incorrect code for this rule: +----------------------------------------- + +.. code-block:: js + + "resource://gre/modules/Console.sys.mjs" + "resource://gre/modules/Log.sys.mjs" + +Examples of correct code for this rule: +--------------------------------------- + +.. code-block:: js + + console.createInstance({ prefix: "Foo" }); diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/use-default-preference-values.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/use-default-preference-values.rst new file mode 100644 index 0000000000..2392709e89 --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/use-default-preference-values.rst @@ -0,0 +1,19 @@ +use-default-preference-values +============================= + +Require providing a second parameter to ``get*Pref`` methods instead of +using a try/catch block. + +Examples of incorrect code for this rule: +----------------------------------------- + +.. code-block:: js + + try { blah = branch.getCharPref('blah'); } catch(e) {} + +Examples of correct code for this rule: +--------------------------------------- + +.. code-block:: js + + blah = branch.getCharPref('blah', true); diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/use-includes-instead-of-indexOf.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/use-includes-instead-of-indexOf.rst new file mode 100644 index 0000000000..bb65ebea2c --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/use-includes-instead-of-indexOf.rst @@ -0,0 +1,21 @@ +use-includes-instead-of-indexOf +=============================== + +Use ``.includes`` instead of ``.indexOf`` to check if something is in an array +or string. + +Examples of incorrect code for this rule: +----------------------------------------- + +.. code-block:: js + + let a = foo.indexOf(bar) >= 0; + let a = foo.indexOf(bar) == -1; + +Examples of correct code for this rule: +--------------------------------------- + +.. code-block:: js + + let a = foo.includes(bar); + let a = foo.indexOf(bar) > 0; diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/use-isInstance.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/use-isInstance.rst new file mode 100644 index 0000000000..7ba4163228 --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/use-isInstance.rst @@ -0,0 +1,41 @@ +use-isInstance +============== + +Prefer ``.isInstance()`` in chrome scripts over the standard ``instanceof`` +operator for DOM interfaces, since the latter will return false when the object +is created from a different context. + +These files are covered: + +- ``*.sys.mjs`` +- ``*.jsm`` +- ``*.xhtml`` with ``there.is.only.xul`` +- ``*.js`` with a heuristic + +Since there is no straightforward way to detect chrome scripts, currently the +linter assumes that any script including the following words are chrome +privileged. This of course may not be sufficient and is open for change: + +- ``ChromeUtils``, but not ``SpecialPowers.ChromeUtils`` +- ``BrowserTestUtils``, ``PlacesUtils`` +- ``document.createXULElement`` +- ``loader.lazyRequireGetter`` +- ``Services.foo``, but not ``SpecialPowers.Services.foo`` + +Examples of incorrect code for this rule: +----------------------------------------- + +.. code-block:: js + + node instanceof Node + text instanceof win.Text + target instanceof this.contentWindow.HTMLAudioElement + +Examples of correct code for this rule: +--------------------------------------- + +.. code-block:: js + + Node.isInstance(node) + win.Text.isInstance(text) + this.contentWindow.HTMLAudioElement.isInstance(target) diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/use-ownerGlobal.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/use-ownerGlobal.rst new file mode 100644 index 0000000000..5d9905fc9f --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/use-ownerGlobal.rst @@ -0,0 +1,20 @@ +use-ownerGlobal +=============== + +Require ``.ownerGlobal`` instead of ``.ownerDocument.defaultView``. + +Examples of incorrect code for this rule: +----------------------------------------- + +.. code-block:: js + + aEvent.target.ownerDocument.defaultView; + this.DOMPointNode.ownerDocument.defaultView.getSelection(); + +Examples of correct code for this rule: +--------------------------------------- + +.. code-block:: js + + aEvent.target.ownerGlobal; + this.DOMPointNode.ownerGlobal.getSelection(); diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/use-returnValue.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/use-returnValue.rst new file mode 100644 index 0000000000..1280703747 --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/use-returnValue.rst @@ -0,0 +1,20 @@ +use-returnValue +=============== + +Warn when idempotent methods are called and their return value is unused. + +Examples of incorrect code for this rule: +----------------------------------------- + +.. code-block:: js + + foo.concat(bar) + baz.concat() + +Examples of correct code for this rule: +--------------------------------------- + +.. code-block:: js + + a = foo.concat(bar) + b = baz.concat() diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/use-services.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/use-services.rst new file mode 100644 index 0000000000..1a57e3da10 --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/use-services.rst @@ -0,0 +1,21 @@ +use-services +============ + +Requires the use of ``Services`` rather than ``Cc[].getService()`` where a +service is already defined in ``Services``. + +Examples of incorrect code for this rule: +----------------------------------------- + +.. code-block:: js + + Cc["@mozilla.org/appshell/window-mediator;1"].getService(Ci.nsIWindowMediator); + Components.classes["@mozilla.org/toolkit/app-startup;1"].getService(Components.interfaces.nsIAppStartup); + +Examples of correct code for this rule: +--------------------------------------- + +.. code-block:: js + + Services.wm.addListener() + Cc["@mozilla.org/uuid-generator;1"].getService(Ci.nsIUUIDGenerator) diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/use-static-import.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/use-static-import.rst new file mode 100644 index 0000000000..9090dd80b7 --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/use-static-import.rst @@ -0,0 +1,21 @@ +use-static-import +================= + +Requires the use of static imports in system ES module files (``.sys.mjs``) +where possible. + +Examples of incorrect code for this rule: +----------------------------------------- + +.. code-block:: js + + const { XPCOMUtils } = ChromeUtils.importESModule("resource://gre/modules/XPCOMUtils.sys.mjs"); + const { XPCOMUtils: foo } = ChromeUtils.importESModule("resource://gre/modules/XPCOMUtils.sys.mjs"); + +Examples of correct code for this rule: +--------------------------------------- + +.. code-block:: js + + import { XPCOMUtils } from "resource://gre/modules/XPCOMUtils.sys.mjs"; + import { XPCOMUtils as foo } from "resource://gre/modules/XPCOMUtils.sys.mjs"; diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/valid-ci-uses.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/valid-ci-uses.rst new file mode 100644 index 0000000000..440d730e05 --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/valid-ci-uses.rst @@ -0,0 +1,42 @@ +valid-ci-uses +============= + +Ensures that interface accesses on ``Ci`` are valid, and property accesses on +``Ci.<interface>`` are also valid. + +This rule requires a full build to run, and is not turned on by default. To run +this rule manually, use: + +.. code-block:: console + + MOZ_OBJDIR=objdir-ff-opt ./mach eslint --rule="mozilla/valid-ci-uses: error" * + +Examples of incorrect code for this rule: +----------------------------------------- + +``nsIFoo`` does not exist. + +.. code-block:: js + + Ci.nsIFoo + +``UNKNOWN_CONSTANT`` does not exist on nsIURIFixup. + +.. code-block:: js + + Ci.nsIURIFixup.UNKNOWN_CONSTANT + +Examples of correct code for this rule: +--------------------------------------- + +``nsIFile`` does exist. + +.. code-block:: js + + Ci.nsIFile + +``FIXUP_FLAG_NONE`` does exist on nsIURIFixup. + +.. code-block:: js + + Ci.nsIURIFixup.FIXUP_FLAG_NONE diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/valid-lazy.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/valid-lazy.rst new file mode 100644 index 0000000000..a7edeeb5fc --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/valid-lazy.rst @@ -0,0 +1,55 @@ +valid-lazy +========== + +Ensures that definitions and uses of properties on the ``lazy`` object are valid. +This rule checks for using unknown properties, duplicated symbols, unused +symbols, and also lazy getter used at top-level unconditionally. + +Examples of incorrect code for this rule: +----------------------------------------- + +.. code-block:: js + + const lazy = {}; + if (x) { + // Unknown lazy member property {{name}} + lazy.bar.foo(); + } + +.. code-block:: js + + const lazy = {}; + ChromeUtils.defineLazyGetter(lazy, "foo", "foo.jsm"); + + // Duplicate symbol foo being added to lazy. + ChromeUtils.defineLazyGetter(lazy, "foo", "foo1.jsm"); + if (x) { + lazy.foo3.bar(); + } + +.. code-block:: js + + const lazy = {}; + // Unused lazy property foo + ChromeUtils.defineLazyGetter(lazy, "foo", "foo.jsm"); + +.. code-block:: js + + const lazy = {}; + ChromeUtils.defineLazyGetter(lazy, "foo", "foo.jsm"); + // Used at top-level unconditionally. + lazy.foo.bar(); + +Examples of correct code for this rule: +--------------------------------------- + +.. code-block:: js + + const lazy = {}; + ChromeUtils.defineLazyGetter(lazy, "foo1", () => {}); + XPCOMUtils.defineLazyModuleGetters(lazy, { foo2: "foo2.jsm" }); + + if (x) { + lazy.foo1.bar(); + lazy.foo2.bar(); + } diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/valid-services-property.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/valid-services-property.rst new file mode 100644 index 0000000000..c6c61abac2 --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/valid-services-property.rst @@ -0,0 +1,30 @@ +valid-services-property +======================= + +Ensures that accesses of properties of items accessed via the ``Services`` +object are valid. + +This rule requires a full build to run, and is not turned on by default. To run +this rule manually, use: + +.. code-block:: console + + MOZ_OBJDIR=objdir-ff-opt ./mach eslint --rule="mozilla/valid-services-property: error" * + +Examples of incorrect code for this rule: +----------------------------------------- + +Assuming ``foo`` is not defined within ``Ci.nsISearchService``. + +.. code-block:: js + + Services.search.foo(); + +Examples of correct code for this rule: +--------------------------------------- + +Assuming ``bar`` is defined within ``Ci.nsISearchService``. + +.. code-block:: js + + Services.search.bar(); diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/valid-services.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/valid-services.rst new file mode 100644 index 0000000000..bd76cb52ac --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/valid-services.rst @@ -0,0 +1,24 @@ +valid-services +============== + +Ensures that accesses of the ``Services`` object are valid. +``Services`` are defined in ``tools/lint/eslint/eslint-plugin-mozilla/lib/services.json`` and can be added by copying from +``<objdir>/xpcom/components/services.json`` after a build. + +Examples of incorrect code for this rule: +----------------------------------------- + +Assuming ``foo`` is not defined within Services. + +.. code-block:: js + + Services.foo.fn(); + +Examples of correct code for this rule: +--------------------------------------- + +Assuming ``bar`` is defined within Services. + +.. code-block:: js + + Services.bar.fn(); diff --git a/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/var-only-at-top-level.rst b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/var-only-at-top-level.rst new file mode 100644 index 0000000000..d21fc1b299 --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-mozilla/rules/var-only-at-top-level.rst @@ -0,0 +1,21 @@ +var-only-at-top-level +===================== + +Marks all var declarations that are not at the top level invalid. + +Examples of incorrect code for this rule: +----------------------------------------- + +.. code-block:: js + + { var foo; } + function() { var bar; } + +Examples of correct code for this rule: +--------------------------------------- + +.. code-block:: js + + var foo; + { let foo; } + function () { let bar; } diff --git a/docs/code-quality/lint/linters/eslint-plugin-spidermonkey-js.rst b/docs/code-quality/lint/linters/eslint-plugin-spidermonkey-js.rst new file mode 100644 index 0000000000..e20c8562b6 --- /dev/null +++ b/docs/code-quality/lint/linters/eslint-plugin-spidermonkey-js.rst @@ -0,0 +1,18 @@ +============================== +Mozilla ESLint SpiderMonkey JS +============================== + +This plugin adds a processor and an environment for the SpiderMonkey JS code. + +Processors +========== + +The processor is used to pre-process all `*.js` files and deals with the macros +that SpiderMonkey uses. + +Environments +============ + +The plugin provides a custom environment for SpiderMonkey's self-hosted code. It +adds all self-hosting functions, error message numbers, and other self-hosting +definitions as global, read-only identifiers. diff --git a/docs/code-quality/lint/linters/eslint.rst b/docs/code-quality/lint/linters/eslint.rst new file mode 100644 index 0000000000..3f409fb688 --- /dev/null +++ b/docs/code-quality/lint/linters/eslint.rst @@ -0,0 +1,226 @@ +ESLint +====== + +`ESLint`__ is a popular linter for JavaScript. The ESLint integration also uses +`Prettier`_ to enforce code formatting. + +.. contents:: + :local: + +Run Locally +----------- + +The mozlint integration of ESLint can be run using mach: + +.. parsed-literal:: + + $ mach lint --linter eslint <file paths> + +Alternatively, omit the ``--linter eslint`` and run all configured linters, which will include +ESLint. + +ESLint also supports the ``--fix`` option to autofix most errors raised from most of the rules. + +See the `Usage guide`_ for more options. + +Custom Configurations +--------------------- + +Our ESLint configuration has a number of custom configurations that define +globals and rules for various code based on the pattern file path and names. + +Using the correct patterns helps ESLint to know about the correct details, so +that you don't get warnings about undefined or unused variables. + +* ``.mjs`` - A module file. +* ``.sys.mjs`` - A system module, this is typically a singleton in the process it is loaded into. +* ``.worker.(m)js`` - A file that is a web worker. + + * Workers that use ctypes should use ``/* eslint-env mozilla/chrome-worker */`` + +* Test files, see the section on :ref:`adding tests <adding-tests>` + + +Understanding Rules and Errors +------------------------------ + +* Only some files are linted, see the :searchfox:`configuration <tools/lint/eslint.yml>` for details. + + * By design we do not lint/format reftests not crashtests as these are specially crafted tests. + +* If you don't understand a rule, you can look it in `eslint.org's rule list`_ for more + information about it. +* For Mozilla specific rules (with the mozilla/ prefix), see these for more details: + + * `eslint-plugin-mozilla`_ + * `eslint-plugin-spidermonkey-js`_ + +.. _eslint_common_issues: + +Enabling new rules and adding plugins +------------------------------------- + +Please see `this page for enabling new rules <eslint/enabling-rules.html>`_. + +Common Issues and How To Solve Them +----------------------------------- + +My editor says that ``mozilla/whatever`` is unknown +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +* Run ``./mach eslint --setup``, and restart your editor. + +My editor doesn't understand a new global I've just added (e.g. to a content file or head.js file) +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +* This is a limitation which is a mixture of our ESLint setup and how we share globals across files. +* Restarting your editor should pick up the new globals. +* You can always double check via ``./mach lint --linter eslint <file path>`` on the command line. + +I am getting a linter error "Unknown Services member property" +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +Make sure to add any new Services to ``tools/lint/eslint/eslint-plugin-mozilla/lib/services.json``. For example by copying from +``<objdir>/xpcom/components/services.json`` after a build. + +.. _adding-tests: + +I'm adding tests, how do I set up the right configuration? +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +Please note we prefer the tests to be in named directories as this makes it +easier to identify the types of tests developers are working with. Additionally, +it is not possible to scope ESLint rules to individual files based on .ini +files without a build step that would break editors, or an expensive loading +cycle. + +* If the directory path of the tests is one of the `known ones`_, then ESLint will + do the right thing for that test type. This is the preferred option. + + * For example placing xpcshell-tests in ``browser/components/foo/test/unit/`` + will set up ESLint correctly. + +* If you really can't match the directory name, e.g. like the + ``browser/base/content/tests/*``, then you'll need to add a new entry in + :searchfox:`.eslintrc-test-paths.js <.eslintrc-test-paths.js>`. + +Please do not add new cases of multiple types of tests within a single directory, +this is `difficult for ESLint to handle`_. Currently this may cause: + +* Rules to be incorrectly applied to the wrong types of test file. +* Extra definitions for globals in tests which means that the no undefined variables + rule does not get triggered in some cases. + +This code should neither be linted nor formatted +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +* If it is a third-party piece of code, please add it to :searchfox:`ThirdPartyPaths.txt <tools/rewriting/ThirdPartyPaths.txt>`. +* If it is a generated file, please add it to :searchfox:`Generated.txt <tools/rewriting/Generated.txt>`. +* If intentionally invalid, please add it to :searchfox:`.eslintignore <.eslintignore>`. + +This code shouldn't be formatted +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +The vast majority of code should be formatted, however we allow some limited +cases where it makes sense, for example: + +* A table in an array where laying it out in a table fashion makes it more readable. +* Other structures or function calls where layout is more readable in a particular format. + +To disable prettier for code like this, ``// prettier-ignore`` may be used on +the line previous to where you want it disabled. +See the `prettier ignore docs`_ for more information. + +I have valid code that is failing the ``no-undef`` rule or can't be parsed +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +* Please do not add this to :searchfox:`.eslintignore <.eslintignore>`. Generally + this can be fixed, if the following tips don't help, please `seek help`_. +* If you are adding a new test directory, see the :ref:`section above <adding-tests>` + +* If you are writing a script loaded into special environment (e.g. frame script) you may need to tell ESLint to use the `environment definitions`_ for each case: + + * ``/* eslint-env mozilla/frame-script */`` + +* I use ``Services.scriptloader.loadSubScript``: + + * ``/* import-globals-from relative/path/to/file.js`` + +Configuration +------------- + +The global configuration file lives in ``topsrcdir/.eslintrc``. This global configuration can be +overridden by including an ``.eslintrc`` in the appropriate subdirectory. For an overview of the +supported configuration, see `ESLint's documentation`_. + +Please keep differences in rules across the tree to a minimum. We want to be consistent to +make it easier for developers. + +Sources +------- + +* :searchfox:`Configuration (YAML) <tools/lint/eslint.yml>` +* :searchfox:`Source <tools/lint/eslint/__init__.py>` + +Builders +-------- + +`Mark Banner (standard8) <https://people.mozilla.org/s?query=standard8>`__ owns +the builders. Questions can also be asked on #lint:mozilla.org on Matrix. + +ESLint (ES) +^^^^^^^^^^^ + +This is a tier-1 task. For test failures the patch causing the +issue should be backed out or the issue fixed. + +Some failures can be fixed with ``./mach eslint --fix path/to/file``. + +For test harness issues, file bugs in Developer Infrastructure :: Lint and Formatting. + +ESLint-build (ES-B) +^^^^^^^^^^^^^^^^^^^ + +This is a tier-2 task that is run once a day at midnight UTC via a cron job. + +It currently runs the ESLint rules plus two additional rules: + +* `valid-ci-uses <eslint-plugin-mozilla/valid-ci-uses.html>`__ +* `valid-services-property <eslint-plugin-mozilla/valid-services-property.html>`__ + +These are two rules that both require build artifacts. + +To run them manually, you can run: + +``MOZ_OBJDIR=objdir-ff-opt ./mach eslint --rule "mozilla/valid-ci-uses: error" --rule "mozilla/valid-services-property: error" *`` + +For test failures, the regression causing bug may be able to be found by: + + * Determining if the file where the error is reported has been changed recently. + * Seeing if an associated ``.idl`` file has been changed. + +If no regressing bug can easily be found, file a bug in the relevant +product/component for the file where the failure is and cc :standard8. + +For test harness issues, file bugs in Developer Infrastructure :: Lint and Formatting. + +.. toctree:: + :hidden: + + eslint-plugin-mozilla + eslint-plugin-spidermonkey-js + +.. __: https://eslint.org/ +.. _Prettier: https://prettier.io/ +.. _Usage guide: ../usage.html +.. _ESLint's documentation: https://eslint.org/docs/user-guide/configuring +.. _eslint.org's rule list: https://eslint.org/docs/rules/ +.. _eslint-plugin-mozilla: eslint-plugin-mozilla.html +.. _eslint-plugin-spidermonkey-js: eslint-plugin-spidermonkey-js.html +.. _informed that it is a module: https://searchfox.org/mozilla-central/rev/9399e5832979755cd340383f4ca4069dd5fc7774/browser/base/content/.eslintrc.js +.. _seek help: ../index.html#getting-help +.. _patterns in .eslintrc.js: https://searchfox.org/mozilla-central/rev/9399e5832979755cd340383f4ca4069dd5fc7774/.eslintrc.js#24-38 +.. _environment definitions: ./eslint-plugin-mozilla/environment.html +.. _known ones: https://searchfox.org/mozilla-central/rev/287583a4a605eee8cd2d41381ffaea7a93d7b987/.eslintrc.js#24-40 +.. _difficult for ESLint to handle: https://bugzilla.mozilla.org/show_bug.cgi?id=1379669 +.. _prettier ignore docs: https://prettier.io/docs/en/ignore.html diff --git a/docs/code-quality/lint/linters/eslint/enabling-rules.rst b/docs/code-quality/lint/linters/eslint/enabling-rules.rst new file mode 100644 index 0000000000..d20a877d4e --- /dev/null +++ b/docs/code-quality/lint/linters/eslint/enabling-rules.rst @@ -0,0 +1,177 @@ +Enabling Rules and Adding Plugins to ESLint +=========================================== + +This guide is intended to give helpful pointers on how to enable rules and add +plugins to our ESLint configuration. + +.. contents:: + :local: + +General Notes +============= + +* Enabling of new rules and adding plugins should happen in agreement with the + `JavaScript Usage, Tools and Style module owner and peers </mots/index.html#javascript-usage-tools-and-style>`_. + +* Enabling of rules for sub-components should also be discussed with the owner + and peers. + + * Generally we wish to harmonize rules across the entire code base, and so + would prefer to avoid specialisms for different sub-components. + * Exceptions may be made on a sub-component basis. + +Enabling a New Rule +=================== + +The general process for enabling new rules is to file a bug under the +``Developer Infrastructure`` product in the ``Lint and Formatting`` component. + +The rule should then be added to the relevant configurations and existing issues +fixed. For large amounts of existing issues, we may do a staged roll-out +as discussed below. + +Options for Roll-Outs +--------------------- + +For rolling out new rules, we prefer that there is a plan and owner for ensuring +the existing failures are resolved over time. They do not always need to be fixed +immediately, but there should be some agreement as to how existing failures +are addressed, so that we do not end up with a large, potentially complicated +set of exclusions, or significant amounts of warnings that never get addressed. + +This is not to say the developer adding the rule needs to be the owner of the +plan, but they should ensure that there is an agreed way forward. + +There are several options available for roll-outs, depending on how many +errors are found and how much work it is to fix existing issues. + +* Fix any issues and enable the rule everywhere + + * This is most suited to cases where there are a small amount of errors which + are easy to fix up-front + +* Enable the rule everywhere, selectively disabling the rule on existing failures + + * This may be appropriate for cases where fixing the failures may take + a bit longer. + +* Enable the rule as a warning + + * This will raise issues as warnings, which will not prevent patches from + landing with issues, but should at least highlight them during code review. + * This may be more appropriate in situations where there are a large amount + of issues that are non-critical, such as preferring use of one method over + another. + +* Enable the rule as an error on passing code, but a warning on code with failures + + * This is a hybrid approach which is suited to cases where there is an issue + that is more critical, and we want to stop new cases making it into the tree, + and highlight the existing cases if the code gets touched. + +The options here are not firmly set, the list should be used as a guide. + +Where to Add +------------ + +New rules should be added in one of the configurations in +:searchfox:`eslint-plugin-mozilla <tools/lint/eslint/eslint-plugin-mozilla/lib/configs>`. + +These will then automatically be applied to the relevant places. +eslint-plugin-mozilla is used by a few projects outside of mozilla-central, +so they will pick up the rule addition when eslint-plugin-mozilla is next released. + +Where existing failures are disabled/turned to warnings, these should be handled +in the :searchfox:`top-level .eslintrc.js file <.eslintrc.js>`, and follow-up bugs +must be filed before landing and referenced in the appropriate sections. The +follow-up bugs should block +`bug 1596191 <https://bugzilla.mozilla.org/show_bug.cgi?id=1596191>`_ + +Adding a New ESLint Plugin +========================== + +License checks +-------------- + +When a new plugin is proposed, it should be checked to ensure that the licenses +of the node module and all dependent node modules are compatible with the Mozilla +code base. Mozilla employees can consult the +`Licensing & Contributor Agreements Runbook <https://mozilla-hub.atlassian.net/l/cp/bgfp6Be7>`_ +for more details. + +A site such as `npmgraph <https://npmgraph.js.org/>`_ can help with checking +licenses. + +When filing the bug or reviewing a patch, it should be stated if the module +has passed the license checks. + +Adding to the Repository +------------------------ + +If the new plugin is going to have rules defined within a configuration within +eslint-plugin-mozilla, then the module should be referenced in the peer +dependencies of +:searchfox:`eslint-plugin-mozilla's package.json<tools/lint/eslint/eslint-plugin-mozilla/package.json>` +file. + +To add the new module to the node system, run: + +.. code-block:: shell + + ./mach npm install --save-exact --save-dev packagename + +We use exact version matching to make it explicit about the version we are using +and when we upgrade the versions. + +The plugin can then be used with ESLint in the +`normal way <https://eslint.org/docs/latest/use/configure/plugins>`_. + +Packaging node_modules +---------------------- + +For our continuous integration (CI) builders, we package ``node_modules`` for +both the top-level directory, and eslint-plugin-mozilla. These are uploaded to +our CI before the patch is released. + +Currently `Mark Banner (standard8) <https://people.mozilla.org/s?query=standard8>`_ +is the only person that does this regularly, and will be automatically added as +a blocking reviewer on patches that touch the relevant ``package.json`` files. + +A Release Engineering team member would likely have permissions to upload the +files as well. + +To upload the files, the process is: + +* Obtain ToolTool credentials for the public tooltool upload space. + + * Download the `taskcluster shell from here <https://github.com/taskcluster/taskcluster/tree/main/clients/client-shell>`_, + if you haven't already. + * Run the following command. This will open a page for you to log in, and + set environment variables for the following commands to use. + +.. code-block:: shell + + eval `TASKCLUSTER_ROOT_URL=https://firefox-ci-tc.services.mozilla.com taskcluster signin -s 'project:releng:services/tooltool/api/upload/public'` + +* Upload the eslint-plugin-mozilla packages: + +.. code-block:: shell + + cd tools/lint/eslint/eslint-plugin-mozilla/ + ./update.sh + <follow the instructions> + +* Upload the top-level packages: + +.. code-block:: shell + + cd .. + ./update.sh + <follow the instructions> + +* Add the changes to the commit that changes ``package.json``. + +The update scripts automatically clean out the ``node_modules`` directories, +removes the ``package-lock.json`` files, and then does a fresh installation. This +helps to ensure a "clean" directory with only the required modules, and an up to +date ``package-lock.json`` file. diff --git a/docs/code-quality/lint/linters/file-perm.rst b/docs/code-quality/lint/linters/file-perm.rst new file mode 100644 index 0000000000..5c3a02fa1b --- /dev/null +++ b/docs/code-quality/lint/linters/file-perm.rst @@ -0,0 +1,42 @@ +File permission +=============== + +This linter verifies if a file has unnecessary permissions. +If a file has execution permissions (+x), file-perm will +generate a warning. + +It will ignore files starting with ``#!`` for types of files +that typically have shebang lines (such as python, node or +shell scripts). + +This linter does not have any affect on Windows. + + +Run Locally +----------- + +This mozlint linter can be run using mach: + +.. parsed-literal:: + + $ mach lint --linter file-perm <file paths> + + +Configuration +------------- + +This linter is enabled on the whole code base. + +This job is configured as `tier 2 <https://wiki.mozilla.org/Sheriffing/Job_Visibility_Policy#Overview_of_the_Job_Visibility_Tiers>`_. + +Autofix +------- + +This linter provides a ``--fix`` option. The python script is doing the change itself. + + +Sources +------- + +* `Configuration (YAML) <https://searchfox.org/mozilla-central/source/tools/lint/file-perm.yml>`_ +* `Source <https://searchfox.org/mozilla-central/source/tools/lint/file-perm/__init__.py>`_ diff --git a/docs/code-quality/lint/linters/file-whitespace.rst b/docs/code-quality/lint/linters/file-whitespace.rst new file mode 100644 index 0000000000..201f10d123 --- /dev/null +++ b/docs/code-quality/lint/linters/file-whitespace.rst @@ -0,0 +1,38 @@ +Trailing whitespaces +==================== + +This linter verifies if a file has: + +* unnecessary trailing whitespaces, +* Windows carriage return, +* empty lines at the end of file, +* if file ends with a newline or not + + +Run Locally +----------- + +This mozlint linter can be run using mach: + +.. parsed-literal:: + + $ mach lint --linter file-whitespace <file paths> + + +Configuration +------------- + +This linter is enabled on most of the code base. + +This job is configured as `tier 2 <https://wiki.mozilla.org/Sheriffing/Job_Visibility_Policy#Overview_of_the_Job_Visibility_Tiers>`_. + +Autofix +------- + +This linter provides a ``--fix`` option. The python script is doing the change itself. + +Sources +------- + +* :searchfox:`Configuration (YAML) <tools/lint/file-whitespace.yml>` +* :searchfox:`Source <tools/lint/file-whitespace/__init__.py>` diff --git a/docs/code-quality/lint/linters/fluent-lint.rst b/docs/code-quality/lint/linters/fluent-lint.rst new file mode 100644 index 0000000000..809adb4a5e --- /dev/null +++ b/docs/code-quality/lint/linters/fluent-lint.rst @@ -0,0 +1,47 @@ +Fluent Lint +=========== + +Fluent lint is a linter for Fluent files (.ftl). Currently, it includes: + +* Checks for invalid typography in messages (e.g. straight single or double quotes). +* Checks for comments layout. +* Checks for identifiers (minimum length, allowed characters). +* Hard-coded brand names. + + +Run Locally +----------- + +The mozlint integration of fluent-lint can be run using mach: + +.. parsed-literal:: + + $ mach lint --linter fluent-lint <file paths> + +Alternatively, omit the ``--linter fluent-lint`` and run all configured linters, which will include +fluent-lint. + + +Run on Taskcluster +------------------ + +The fluent-lint job shows up as text(fluent) in the linting job. It should run automatically if +changes are made to fluent (ftl) files. + + +Configuration +------------- + +The main configuration file is found in :searchfox:`tools/lint/fluent-lint/exclusions.yml`. This provides +a way of excluding identifiers or files from checking. In general, exclusions are only to be +used for identifiers that are generated programmatically, but unfortunately, there are other +exclusions that are required for historical reasons. In almost all cases, it should *not* be +necessary to add new exclusions to this file. + + +Sources +------- + +* :searchfox:`Configuration (YAML) <tools/lint/fluent-lint.yml>` +* :searchfox:`Source <tools/lint/fluent-lint/__init__.py>` +* :searchfox:`Test <tools/lint/test/test_fluent_lint.py>` diff --git a/docs/code-quality/lint/linters/l10n.rst b/docs/code-quality/lint/linters/l10n.rst new file mode 100644 index 0000000000..de4ce990c8 --- /dev/null +++ b/docs/code-quality/lint/linters/l10n.rst @@ -0,0 +1,45 @@ +L10n +==== + +The l10n linter checks for mistakes and problems in the localizable files. +Most of the code lives inside the +`compare-locales <https://pypi.org/project/compare-locales/>`_ +package, and is shipping as the ``moz-l10n-lint`` command. + +The linter checks for fundamental issues like parsing errors, but it also +finds more subtle mistakes like duplicated messages. It also warns if you're +trying to change a string without changing the ID, or to add a string that's +still in use in a stable channel with a different value. + +The warnings on string ID changes get reported on phabricator, but they're +not making the build fail. To find out when to change IDs and when not to, +read the :ref:`Lifecycle & Workflow <Localization>` section in the +localization documentation. + +Run Locally +----------- + +The can be run using mach: + +.. parsed-literal:: + + $ mach lint --linter l10n <file paths> + +Alternatively, omit the ``--linter l10n`` and run all configured linters, which +will include the l10n linter. + + +Updating the Reference +---------------------- + +The linter checks out the cross-channel localization files into your +``.mozbuild`` state directory. By default this is updated automatically after +48 hours. There might be new strings anyway, if you want to ensure an +updated clone, remove the marker file in +``~/.mozbuild/gecko-strings/.hg/l10n_pull_marker``. + +Sources +------- + +* :searchfox:`Configuration (YAML) <tools/lint/l10n.yml>` +* :searchfox:`Source <tools/lint/python/l10n_lint.py>` diff --git a/docs/code-quality/lint/linters/license.rst b/docs/code-quality/lint/linters/license.rst new file mode 100644 index 0000000000..0533333218 --- /dev/null +++ b/docs/code-quality/lint/linters/license.rst @@ -0,0 +1,39 @@ +License +======= + +This linter verifies if a file has a known license header. + +By default, Firefox uses MPL-2 license with the `appropriate headers <https://www.mozilla.org/en-US/MPL/headers/>`_. +In some cases (thirdpardy code), a file might have a different header file. +If this is the case, one of the significant line of the header should be listed in the list `of valid licenses +<https://searchfox.org/mozilla-central/source/tools/lint/license/valid-licenses.txt>`_. + +Run Locally +----------- + +This mozlint linter can be run using mach: + +.. parsed-literal:: + + $ mach lint --linter license <file paths> + + +Configuration +------------- + +This linter is enabled on most of the whole code base. + +Autofix +------- + +This linter provides a ``--fix`` option. The python script is doing the change itself +and will use the right header MPL-2 header depending on the language. +It will add the license at the right place in case the file is a script (ie starting with ``!#`` +or a XML file ``<?xml>``). + + +Sources +------- + +* `Configuration (YAML) <https://searchfox.org/mozilla-central/source/tools/lint/license.yml>`_ +* `Source <https://searchfox.org/mozilla-central/source/tools/lint/license/__init__.py>`_ diff --git a/docs/code-quality/lint/linters/lintpref.rst b/docs/code-quality/lint/linters/lintpref.rst new file mode 100644 index 0000000000..ca19089172 --- /dev/null +++ b/docs/code-quality/lint/linters/lintpref.rst @@ -0,0 +1,32 @@ +Lintpref +======== + +The lintpref linter is a simple linter for libpref files to check for duplicate +entries between :searchfox:`modules/libpref/init/all.js` and +:searchfox:`modules/libpref/init/StaticPrefList.yaml`. If a duplicate is found, +lintpref will raise an error and emit the ``all.js`` line where you can find +the duplicate entry. + + +Running Locally +--------------- + +The linter can be run using mach: + + .. parsed-literal:: + + $ mach lint --linter lintpref + + +Fixing Lintpref Errors +---------------------- + +In most cases, duplicate entries should be avoided and the duplicate removed +from ``all.js``. If for any reason a pref should exist in both files, the pref +should be added to ``IGNORE_PREFS`` in :searchfox:`tools/lint/libpref/__init__.py`. + +Sources +------- + +* :searchfox:`Configuration (YAML) <tools/lint/lintpref.yml>` +* :searchfox:`Source <tools/lint/libpref/__init__.py>` diff --git a/docs/code-quality/lint/linters/mingw-capitalization.rst b/docs/code-quality/lint/linters/mingw-capitalization.rst new file mode 100644 index 0000000000..e6c51a4d14 --- /dev/null +++ b/docs/code-quality/lint/linters/mingw-capitalization.rst @@ -0,0 +1,28 @@ +MinGW capitalization +==================== + +This linter verifies that Windows include file are lowercase. +It might break the mingw build otherwise. + + +Run Locally +----------- + +This mozlint linter can be run using mach: + +.. parsed-literal:: + + $ mach lint --linter mingw-capitalization <file paths> + + +Configuration +------------- + +This linter is enabled on the whole code base except WebRTC + + +Sources +------- + +* :searchfox:`Configuration (YAML) <tools/lint/mingw-capitalization.yml>` +* :searchfox:`Source <tools/lint/cpp/mingw-capitalization.py>` diff --git a/docs/code-quality/lint/linters/perfdocs.rst b/docs/code-quality/lint/linters/perfdocs.rst new file mode 100644 index 0000000000..1ec2e33e68 --- /dev/null +++ b/docs/code-quality/lint/linters/perfdocs.rst @@ -0,0 +1,84 @@ +PerfDocs +======== + +`PerfDocs`_ is a tool that checks to make sure all performance tests are documented in tree. + +At the moment, it is only used for this documentation verification, but in the future it will also auto-generate documentation from these descriptions that will be displayed in the source-docs documentation page (rather than the wiki, which is where they currently reside). + +Run Locally +----------- + +The mozlint integration of PerfDocs can be run using mach: + +.. parsed-literal:: + + $ mach lint --linter perfdocs . + + +Configuration +------------- + +There are no configuration options available for this linter. It scans the full source tree under ``testing``, looking for folders named ``perfdocs`` and then validates their content. This has only been implemented for Raptor so far, but Talos will be added in the future. We also hope to expand this to search outside the ``testing`` directory. + +The ``perfdocs`` folders, there needs to be an ``index.rst`` file and it needs to contain the string ``{documentation}`` in some location in the file which is where the test documentation will be placed. The folders must also have a ``config.yml`` file following this schema: + +.. code-block:: python + + CONFIG_SCHEMA = { + "type": "object", + "properties": { + "name": {"type": "string"}, + "manifest": {"type": "string"}, + "suites": { + "type": "object", + "properties": { + "suite_name": { + "type": "object", + "properties": { + "tests": { + "type": "object", + "properties": { + "test_name": {"type": "string"}, + } + }, + "description": {"type": "string"}, + }, + "required": [ + "description" + ] + } + } + } + }, + "required": [ + "name", + "manifest", + "suites" + ] + } + +Here is an example of a configuration file for the Raptor framework: + +.. parsed-literal:: + + name: raptor + manifest: testing/raptor/raptor/raptor.toml + suites: + desktop: + description: "Desktop tests." + tests: + raptor-tp6: "Raptor TP6 tests." + mobile: + description: "Mobile tests" + benchmarks: + description: "Benchmark tests." + tests: + wasm: "All wasm tests." + +Note that there needs to be a FrameworkGatherer implemented for the framework being documented since each of them may have different ways of parsing test manifests for the tests. See `RaptorGatherer <https://searchfox.org/mozilla-central/source/tools/lint/perfdocs/framework_gatherers.py>`_ for an example gatherer that was implemented for Raptor. + +Sources +------- + +* `Configuration <https://searchfox.org/mozilla-central/source/tools/lint/perfdocs.yml>`__ +* `Source <https://searchfox.org/mozilla-central/source/tools/lint/perfdocs>`__ diff --git a/docs/code-quality/lint/linters/rejected-words.rst b/docs/code-quality/lint/linters/rejected-words.rst new file mode 100644 index 0000000000..9afe7df27e --- /dev/null +++ b/docs/code-quality/lint/linters/rejected-words.rst @@ -0,0 +1,28 @@ +Rejected words +============== + +Reject some words we don't want to use in the code base for various reasons. + +Run Locally +----------- + +The mozlint integration of codespell can be run using mach: + +.. parsed-literal:: + + $ mach lint --linter rejected-words <file paths> + + +Configuration +------------- + +This linter is enabled on the whole code base. Issues existing in the code base +are listed in the exclude list in the :searchfox:`rejected-words.yml +<tools/lint/rejected-words.yml>` file. + +New words can be added in the `payload` section. + +Sources +------- + +* :searchfox:`Configuration (YAML) <tools/lint/rejected-words.yml>` diff --git a/docs/code-quality/lint/linters/rstlinter.rst b/docs/code-quality/lint/linters/rstlinter.rst new file mode 100644 index 0000000000..46a68d5849 --- /dev/null +++ b/docs/code-quality/lint/linters/rstlinter.rst @@ -0,0 +1,32 @@ +RST Linter +========== + +`rstcheck`_ is a popular linter for restructuredtext. + + +Run Locally +----------- + +The mozlint integration of rst linter can be run using mach: + +.. parsed-literal:: + + $ mach lint --linter rst <file paths> + + +Configuration +------------- + +All directories will have rst linter run against them. +If you wish to exclude a subdirectory of an included one, you can add it to the ``exclude`` +directive. + + +.. _rstcheck: https://github.com/myint/rstcheck + + +Sources +------- + +* :searchfox:`Configuration (YAML) <tools/lint/rst.yml>` +* :searchfox:`Source <tools/lint/rst/__init__.py>` diff --git a/docs/code-quality/lint/linters/ruff.rst b/docs/code-quality/lint/linters/ruff.rst new file mode 100644 index 0000000000..359e8dcbf6 --- /dev/null +++ b/docs/code-quality/lint/linters/ruff.rst @@ -0,0 +1,44 @@ +Ruff +==== + +`Ruff <https://github.com/charliermarsh/ruff>`_ is an extremely fast Python +linter and formatter, written in Rust. It can process all of mozilla-central in +under a second, and implements rule sets from a large array of Python linters +and formatters, including: + +* flake8 (pycodestyle, pyflakes and mccabe) +* isort +* pylint +* pyupgrade +* and many many more! + +Run Locally +----------- + +The mozlint integration of ruff can be run using mach: + +.. parsed-literal:: + + $ mach lint --linter ruff <file paths> + + +Configuration +------------- + +Ruff is configured in the root `pyproject.toml`_ file. Additionally, ruff will +pick up any ``pyproject.toml`` or ``ruff.toml`` files in subdirectories. The +settings in these files will only apply to files contained within these +subdirs. For more details on configuration discovery, see the `configuration +documentation`_. + +For a list of options, see the `settings documentation`_. + +Sources +------- + +* `Configuration (YAML) <https://searchfox.org/mozilla-central/source/tools/lint/ruff.yml>`_ +* `Source <https://searchfox.org/mozilla-central/source/tools/lint/python/ruff.py>`_ + +.. _pyproject.toml: https://searchfox.org/mozilla-central/source/pyproject.toml +.. _configuration documentation: https://beta.ruff.rs/docs/configuration/ +.. _settings documentation: https://beta.ruff.rs/docs/settings/ diff --git a/docs/code-quality/lint/linters/rustfmt.rst b/docs/code-quality/lint/linters/rustfmt.rst new file mode 100644 index 0000000000..eb7e75fa6b --- /dev/null +++ b/docs/code-quality/lint/linters/rustfmt.rst @@ -0,0 +1,33 @@ +Rustfmt +======= + +`rustfmt <https://github.com/rust-lang/rustfmt>`__ is the tool for Rust coding style. + +Run Locally +----------- + +The mozlint integration of rustfmt can be run using mach: + +.. parsed-literal:: + + $ mach lint --linter rustfmt <file paths> + + +Configuration +------------- + +To enable rustfmt on new directory, add the path to the include +section in the :searchfox:`rustfmt.yml <tools/lint/rustfmt.yml>` file. + + +Autofix +------- + +Rustfmt is reformatting the code by default. To highlight the results, we are using +the ``--check`` option. + +Sources +------- + +* :searchfox:`Configuration (YAML) <tools/lint/rustfmt.yml>` +* :searchfox:`Source <tools/lint/rust/__init__.py>` diff --git a/docs/code-quality/lint/linters/stylelint.rst b/docs/code-quality/lint/linters/stylelint.rst new file mode 100644 index 0000000000..1e8b6c7ff9 --- /dev/null +++ b/docs/code-quality/lint/linters/stylelint.rst @@ -0,0 +1,77 @@ +Stylelint +========= + +`Stylelint`__ is a popular linter for CSS. + +Run Locally +----------- + +The mozlint integration of Stylelint can be run using mach: + +.. parsed-literal:: + + $ mach lint --linter stylelint <file paths> + +Alternatively, omit the ``--linter stylelint`` and run all configured linters, which will include +Stylelint. + +Stylelint also supports the ``--fix`` option to autofix most errors raised from most of the rules. + +See the `Usage guide`_ for more options. + +Understanding Rules and Errors +------------------------------ + +* Only some files are linted, see the :searchfox:`configuration <tools/lint/stylelint.yml>` for details. + + * By design we do not lint/format reftests not crashtests as these are specially crafted tests. + +* If you don't understand a rule, you can look it in `stylelint.io's rule list`_ for more + information about it. + +Common Issues and How To Solve Them +----------------------------------- + +This code should neither be linted nor formatted +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +* If it is a third-party piece of code, please add it to :searchfox:`ThirdPartyPaths.txt <tools/rewriting/ThirdPartyPaths.txt>`. +* If it is a generated file, please add it to :searchfox:`Generated.txt <tools/rewriting/Generated.txt>`. +* If intentionally invalid, please add it to :searchfox:`.stylelintignore <.stylelintignore>`. + +Configuration +------------- + +The global configuration file lives in ``topsrcdir/.stylelintrc.js``. +For an overview of the supported configuration, see `Stylelint's documentation`_. + +Please keep differences in rules across the tree to a minimum. We want to be consistent to +make it easier for developers. + +Sources +------- + +* :searchfox:`Configuration (YAML) <tools/lint/stylelint.yml>` +* :searchfox:`Source <tools/lint/stylelint/__init__.py>` + +Builders +-------- + +`Gijs Kruitbosch (gijs) <https://people.mozilla.org/s?query=gijs>`__ owns +the builders. Questions can also be asked on #lint:mozilla.org on Matrix. + +Stylelint task +^^^^^^^^^^^^^^ + +This is a tier-1 task. For test failures the patch causing the +issue should be backed out or the issue fixed. + +Some failures can be fixed with ``./mach lint -l stylelint --fix path/to/file``. + +For test harness issues, file bugs in Developer Infrastructure :: Lint and Formatting. + + +.. __: https://stylelint.io/ +.. _Usage guide: ../usage.html +.. _Stylelint's documentation: https://stylelint.io/user-guide/configure/ +.. _stylelint.io's rule list: https://stylelint.io/user-guide/rules/ diff --git a/docs/code-quality/lint/linters/test-manifest-toml.rst b/docs/code-quality/lint/linters/test-manifest-toml.rst new file mode 100644 index 0000000000..1060b2756b --- /dev/null +++ b/docs/code-quality/lint/linters/test-manifest-toml.rst @@ -0,0 +1,35 @@ +Test Manifest TOML +================== + +This linter verifies syntax for ManifestParser TOML files. + +Run Locally +----------- + +This mozlint linter can be run using mach: + +.. parsed-literal:: + + $ mach lint --linter test-manifest-toml <file paths> + + +Configuration +------------- + +The configuration excludes all non-ManifestParser TOML files (as well as +generated TOML manifests). + +Sources +------- + +* `Configuration (YAML) <https://searchfox.org/mozilla-central/source/tools/lint/test-manifest-toml.yml>`_ +* `Source <https://searchfox.org/mozilla-central/source/tools/lint/test-manifest-toml/__init__.py>`_ + +Errors Detected +--------------- +* Invalid TOML +* Missing DEFAULT section (fixable) +* Sections not in alphabetical order (fixable) +* Section name not double quoted (fixable) +* Disabling a path by commenting out the section +* Conditional contains explicit || diff --git a/docs/code-quality/lint/linters/trojan-source.rst b/docs/code-quality/lint/linters/trojan-source.rst new file mode 100644 index 0000000000..250bdd9afe --- /dev/null +++ b/docs/code-quality/lint/linters/trojan-source.rst @@ -0,0 +1,34 @@ +Trojan Source +============= + +This linter verifies if a change is using some invalid unicode. + +The goal of this linter is to identify some potential usage of this +technique: + +https://trojansource.codes/ + +The code is inspired by the Red Hat script published: + +https://access.redhat.com/security/vulnerabilities/RHSB-2021-007#diagnostic-tools + +Run Locally +----------- + +This mozlint linter can be run using mach: + +.. parsed-literal:: + + $ mach lint --linter trojan-source <file paths> + + +Configuration +------------- + +This linter is enabled on most of the code base on C/C++, Python and Rust. + +Sources +------- + +* `Configuration (YAML) <https://searchfox.org/mozilla-central/source/tools/lint/trojan-source.yml>`_ +* `Source <https://searchfox.org/mozilla-central/source/tools/lint/trojan-source/__init__.py>`_ diff --git a/docs/code-quality/lint/linters/yamllint.rst b/docs/code-quality/lint/linters/yamllint.rst new file mode 100644 index 0000000000..e148a6aace --- /dev/null +++ b/docs/code-quality/lint/linters/yamllint.rst @@ -0,0 +1,31 @@ +yamllint +======== + +`yamllint <https://github.com/adrienverge/yamllint>`__ is a linter for YAML files. + + +Run Locally +----------- + +The mozlint integration of yamllint can be run using mach: + +.. parsed-literal:: + + $ mach lint --linter yaml <file paths> + +Alternatively, omit ``--linter yaml`` to run all configured linters, including +yamllint. + + +Configuration +------------- + +To enable yamllint on a new directory, add the path to the include section in +the :searchfox:`yaml.yml <tools/lint/yaml.yml>` file. + + +Sources +------- + +* :searchfox:`Configuration (YAML) <tools/lint/yaml.yml>` +* :searchfox:`Source <tools/lint/yamllint_/__init__.py>` diff --git a/docs/code-quality/lint/mozlint.rst b/docs/code-quality/lint/mozlint.rst new file mode 100644 index 0000000000..48a6a78a6f --- /dev/null +++ b/docs/code-quality/lint/mozlint.rst @@ -0,0 +1,23 @@ +MozLint +======= + +Linters are used in mozilla-central to help enforce coding style and avoid bad practices. +Due to the wide variety of languages in use, this is not always an easy task. +In addition, linters should be runnable from editors, from the command line, from review tools +and from continuous integration. It's easy to see how the complexity of running all of these +different kinds of linters in all of these different places could quickly balloon out of control. + +``Mozlint`` is a library that accomplishes several goals: + +1. It provides a standard method for adding new linters to the tree, which can be as easy as + defining a config object in a ``.yml`` file. This helps keep lint related code localized, and + prevents different teams from coming up with their own unique lint implementations. +2. It provides a streamlined interface for running all linters at once. Instead of running N + different lint commands to test your patch, a single ``mach lint`` command will automatically run + all applicable linters. This means there is a single API surface that other tools can use to + invoke linters. +3. With a simple taskcluster configuration, Mozlint provides an easy way to execute all these jobs + at review phase. + +``Mozlint`` isn't designed to be used directly by end users. Instead, it can be consumed by things +like mach, phabricator and taskcluster. diff --git a/docs/code-quality/lint/usage.rst b/docs/code-quality/lint/usage.rst new file mode 100644 index 0000000000..49e0ac81cc --- /dev/null +++ b/docs/code-quality/lint/usage.rst @@ -0,0 +1,138 @@ +Running Linters Locally +======================= + +Using the Command Line +---------------------- + +You can run all the various linters in the tree using the ``mach lint`` command. Simply pass in the +directory or file you wish to lint (defaults to current working directory): + +.. parsed-literal:: + + ./mach lint path/to/files + +Multiple paths are allowed: + +.. parsed-literal:: + + ./mach lint path/to/foo.js path/to/bar.py path/to/dir + +To force execution on a directory that would otherwise be excluded: + +.. parsed-literal:: + + ./mach lint -n path/in/the/exclude/list + +``Mozlint`` will automatically determine which types of files exist, and which linters need to be run +against them. For example, if the directory contains both JavaScript and Python files then mozlint +will automatically run both ESLint and Flake8 against those files respectively. + +To restrict which linters are invoked manually, pass in ``-l/--linter``: + +.. parsed-literal:: + + ./mach lint -l eslint path/to/files + +You can see a list of the available linters by running: + +.. parsed-literal:: + + ./mach lint --list + +Finally, ``mozlint`` can lint the files touched by outgoing revisions or the working directory using +the ``-o/--outgoing`` and ``-w/--workdir`` arguments respectively. These work both with mercurial and +git. In the case of ``--outgoing``, the default remote repository the changes would be pushed to is +used as the comparison. If desired, a remote can be specified manually. In git, you may only want to +lint staged commits from the working directory, this can be accomplished with ``--workdir=staged``. +Examples: + +.. parsed-literal:: + + ./mach lint --workdir + ./mach lint --workdir=staged + ./mach lint --outgoing + ./mach lint --outgoing origin/master + ./mach lint -wo + +.. _lint-vcs-hook: + +Using a VCS Hook +---------------- + +There are also both pre-commit and pre-push version control hooks that work in +either hg or git. To enable a pre-push hg hook, add the following to hgrc: + +.. parsed-literal:: + + [hooks] + pre-push.lint = python:./tools/lint/hooks.py:hg + + +To enable a pre-commit hg hook, add the following to hgrc: + +.. parsed-literal:: + + [hooks] + pretxncommit.lint = python:./tools/lint/hooks.py:hg + + +To enable a pre-push git hook, run the following command: + +.. parsed-literal:: + + $ ln -s ../../tools/lint/hooks.py .git/hooks/pre-push + + +To enable a pre-commit git hook, run the following command: + +.. parsed-literal:: + + $ ln -s ../../tools/lint/hooks.py .git/hooks/pre-commit + +Note that the symlink will be interpreted as ``.git/hooks/../../tools/lint/hooks.py``. + +Automatically Fixing Lint Errors +-------------------------------- + +``Mozlint`` has a best-effort ability to fix lint errors: + +.. parsed-literal:: + + $ ./mach lint --fix + +Not all linters support fixing, and even the ones that do can not usually fix +all types of errors. Any errors that cannot be automatically fixed, will be +printed to stdout like normal. In that case, you can also fix errors manually: + +.. parsed-literal:: + + $ ./mach lint --edit + +This requires the $EDITOR environment variable be defined. For most editors, +this will simply open each file containing errors one at a time. For vim (or +neovim), this will populate the `quickfix list`_ with the errors. + +The ``--fix`` and ``--edit`` arguments can be combined, in which case any +errors that can be fixed automatically will be, and the rest will be opened +with your $EDITOR. + +Editor Integration +================== + +.. note:: + + See details on `how to set up your editor here </contributing/editor.html#editor-ide-integration>`_ + +Editor integrations are highly recommended for linters, as they let you see +errors in real time, and can help you fix issues before you compile or run tests. + +Although mozilla-central does not currently have an integration available for +`./mach lint`, there are various integrations available for some of the major +linting tools that we use: + +* `ESLint`_ +* `Black (Python)`_ + +.. _quickfix list: http://vimdoc.sourceforge.net/htmldoc/quickfix.html +.. _ESLint: https://eslint.org/docs/user-guide/integrations#editors +.. _Black (Python): https://black.readthedocs.io/en/stable/editor_integration.html diff --git a/docs/code-quality/static-analysis/existing.rst b/docs/code-quality/static-analysis/existing.rst new file mode 100644 index 0000000000..9b20650ad0 --- /dev/null +++ b/docs/code-quality/static-analysis/existing.rst @@ -0,0 +1,245 @@ +Existing Infrastructure and Analysis +==================================== + +This document is about how Static Analysis occurs at Mozilla: the Firefox-specific and general llvm clang-tidy checks that are run on submissions in Phabricator and how to run them locally. For information about how to develop your own static analysis checks, please see `Writing New Firefox-Specific Checks <../static-analysis/writing-new/index.html>`_. + +For linting, please see the `linting documentation <../lint/index.html>`_. + +For reviews, use the `#static-analysis-reviewers review group <https://phabricator.services.mozilla.com/project/view/120/>`__. +Ask questions on `#static-analysis:mozilla.org <https://chat.mozilla.org/#/room/#static-analysis:mozilla.org>`__. + + +Clang-Tidy static analysis +-------------------------- + +As explained earlier, our current static-analysis infrastructure is based on +`clang-tidy <http://clang.llvm.org/extra/clang-tidy/>`__. The checkers that +we use are split into 3 categories: + +#. :searchfox:`Firefox specific checkers <build/clang-plugin>`. They detect incorrect Gecko programming + patterns which could lead to bugs or security issues. +#. `Clang-tidy checkers <https://clang.llvm.org/extra/clang-tidy/checks/list.html>`_. They aim to suggest better programming practices + and to improve memory efficiency and performance. +#. `Clang-analyzer checkers <https://clang-analyzer.llvm.org/>`_. These checks are more advanced, for example + some of them can detect dead code or memory leaks, but as a typical + side effect they have false positives. Because of that, we have + disabled them for now, but will enable some of them in the near + future. + +In order to simplify the process of static-analysis we have focused on +integrating this process with Phabricator and mach. A list of some +checkers that are used during automated scan can be found +:searchfox:`here <tools/clang-tidy/config.yaml>`. + +Static analysis at review phase +------------------------------- + +We created a TaskCluster bot that runs clang static analysis on every +patch submitted to Phabricator. It then quickly reports any code defects +directly on the review platform, thus preventing bad patches from +landing until all their defects are fixed. Currently, its feedback is +posted in about 10 minutes after a patch series is published on the +review platform. + +As part of the process, the various linting jobs are also executed +using try. This can be also used to add new jobs, see: :ref:`attach-job-review`. +An example of automated review can be found `on +phabricator <https://phabricator.services.mozilla.com/D2066>`__. + + +./mach static-analysis +---------------------- + +The ``./mach static-analysis`` command is supported on all Firefox built platforms. During the first run it +automatically installs all of its dependencies, such as the clang-tidy +executable, in the .mozbuild folder thus making it very easy to use. The +resources that are used are provided by toolchain artifacts clang-tidy +target. + +This is used through ``mach static-analysis`` command that has the +following parameters: + +- ``check`` - Runs the checks using the installed helper tool from + ~/.mozbuild. +- ``--checks, -c`` - Checks to enabled during the scan. The checks + enabled + :searchfox:`in the yaml file <tools/clang-tidy/config.yaml>` + are used by default. +- ``--fix, -f`` - Try to autofix errors detected by the checkers. + Depending on the checker, this option might not do anything. + The list of checkers with autofix can be found on the `clang-tidy website <https://clang.llvm.org/extra/clang-tidy/checks/list.html>`__. +- ``--header-filter, -h-f`` - Regular expression matching the names of + the headers to output diagnostic from. Diagnostic from the main file + of each translation unit are always displayed. + +As an example we run static-analysis through mach on +``dom/presentation/Presentation.cpp`` with +``google-readability-braces-around-statements`` check and autofix we +would have: + +.. code-block:: shell + + ./mach static-analysis check --checks="-*, google-readability-braces-around-statements" --fix dom/presentation/Presentation.cpp + +If you want to use a custom clang-tidy binary this can be done by using +the ``install`` subcommand of ``mach static-analysis``, but please note +that the archive that is going to be used must be compatible with the +directory structure clang-tidy from toolchain artifacts. + +.. code-block:: shell + + ./mach static-analysis install clang.tar.gz + + +Regression Testing +------------------ + +In order to prevent regressions in our clang-tidy based static analysis, +we have created a +:searchfox:`task <taskcluster/ci/static-analysis-autotest/kind.yml>` +on automation. This task runs on each commit and launches a test suite +that is integrated into mach. + +The test suite implements the following: + +- Downloads the necessary clang-tidy artifacts. +- Reads the + :searchfox:`configuration <tools/clang-tidy/config.yaml>` + file. +- For each checker reads the test file plus the expected result. A + sample of test and expected result can be found + :searchfox:`in the test file <tools/clang-tidy/test/clang-analyzer-deadcode.DeadStores.cpp>` + and + :searchfox:`the json file <tools/clang-tidy/test/clang-analyzer-deadcode.DeadStores.json>`. + +This testing suit can be run locally by doing the following: + +.. code-block:: shell + + ./mach static-analysis autotest + +If we want to test only a specific checker, let's say +modernize-raw-string-literal, we can run: + +.. code-block:: shell + + ./mach static-analysis autotest modernize-raw-string-literal + +If we want to add a new checker we need to generate the expected result +file, by doing: + +.. code-block:: shell + + ./mach static-analysis autotest modernize-raw-string-literal -d + + +Build-time static-analysis +-------------------------- + +If you want to build with the Firefox Clang plug-in +(located in ``/build/clang-plugin`` and associated with +``MOZ_CLANG_PLUGIN`` and the attributes in ``/mfbt/Attributes.h``) +just add ``--enable-clang-plugin`` to your mozconfig! +If you want to also have our experimental checkers that will produce ``warnings`` as +diagnostic messages also add ``--enable-clang-plugin-alpha``. +This requires to build Firefox using Clang. + +Configuring the build environment +--------------------------------- + +Once you have your Clang build in place, you will need to set up tools +to use it. +A full working .mozconfig for the desktop browser is: + +.. code-block:: shell + + . $topsrcdir/browser/config/mozconfig + mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/obj-ff-dbg + + ac_add_options --enable-debug + +Attempts to use ``ccache`` will likely result in failure to compile. It +is also necessary to avoid optimized builds, as these will modify macros +which will result in many false positives. + +At this point, your Firefox build environment should be configured to +compile via the Clang static analyzer! + + +Performing scanning builds +-------------------------- + +It is not enough to simply start the build like normal. Instead, you +need to run the build through a Clang utility script which will keep +track of all produced analysis and consolidate it automatically. + +Reports are published daily on +`https://sylvestre.ledru.info/reports/fx-scan-build/ <http://sylvestre.ledru.info/reports/fx-scan-build/>`__ +Many of the defects reported as sources for Good First Bug. + +That script is scan-build. You can find it in +``$clang_source/tools/scan-build/scan-build``. + +Try running your build through ``scan-build``: + +.. code-block:: shell + + $ cd /path/to/mozilla/source + + # Blow away your object directory because incremental builds don't make sense + $ rm -rf obj-dir + + # To start the build: + scan-build --show-description ./mach build -v + + # The above should execute without any errors. However, it should take longer than + # normal because all compilation will be executing through Clang's static analyzer, + # which adds overhead. + +If things are working properly, you should see a bunch of console spew, +just like any build. + +The first time you run scan-build, CTRL+C after a few files are +compiled. You should see output like: + +.. code-block:: shell + + scan-build: 3 bugs found. + scan-build: Run 'scan-view /Users/gps/tmp/mcsb/2011-12-15-3' to examine bug reports. + +If you see a message like: + +.. code-block:: shell + + scan-build: Removing directory '/var/folders/s2/zc78dpsx2rz6cpc_21r9g5hr0000gn/T/scan-build-2011-12-15-1' because it contains no reports. + +Either no static analysis results were available yet or your environment +is not configured properly. + +By default, ``scan-build`` writes results to a folder in a +pseudo-temporary location. You can control where results go by passing +the ``-o /path/to/output`` arguments to ``scan-build``. + +You may also want to run ``scan-build --help`` to see all the options +available. For example, it is possible to selectively enable and disable +individual analyzers. + + +Analyzing the output +-------------------- + +Once the build has completed, ``scan-build`` will produce a report +summarizing all the findings. This is called ``index.html`` in the +output directory. You can run ``scan-view`` (from +``$clang_source/tools/scan-view/scan-view``) as ``scan-build's`` output +suggests; this merely fires up a local HTTP server. Or you should be +able to open the ``index.html`` directly with your browser. + + +False positives +--------------- + +By definition, there are currently false positives in the static +analyzer. A lot of these are due to the analyzer having difficulties +following the relatively complicated error handling in various +preprocessor macros. diff --git a/docs/code-quality/static-analysis/index.rst b/docs/code-quality/static-analysis/index.rst new file mode 100644 index 0000000000..595eab363d --- /dev/null +++ b/docs/code-quality/static-analysis/index.rst @@ -0,0 +1,30 @@ +Static Analysis +=============== + +Static Analysis is running an analysis of the source code without actually executing the code. For the most part, at Mozilla static analysis refers to the stuff we do with `clang-tidy <http://clang.llvm.org/extra/clang-tidy/>`__. It uses +checkers in order to prevent different programming errors present in the +code. The checkers that we use are split into 3 categories: + +#. :searchfox:`Firefox specific checkers <build/clang-plugin>`. They detect incorrect Gecko programming + patterns which could lead to bugs or security issues. +#. `Clang-tidy checkers <https://clang.llvm.org/extra/clang-tidy/checks/list.html>`_. They aim to suggest better programming practices + and to improve memory efficiency and performance. +#. `Clang-analyzer checkers <https://clang-analyzer.llvm.org/>`_. These checks are more advanced, for example + some of them can detect dead code or memory leaks, but as a typical + side effect they have false positives. Because of that, we have + disabled them for now, but will enable some of them in the near + future. + +In order to simplify the process of static-analysis we have focused on +integrating this process with Phabricator and mach. A list of some +checkers that are used during automated scan can be found +:searchfox:`here <tools/clang-tidy/config.yaml>`. + +This documentation is split into two parts: + +.. toctree:: + :maxdepth: 1 + :glob: + + existing.rst + writing-new/index.rst diff --git a/docs/code-quality/static-analysis/writing-new/adding-a-check.rst b/docs/code-quality/static-analysis/writing-new/adding-a-check.rst new file mode 100644 index 0000000000..ec3bc030af --- /dev/null +++ b/docs/code-quality/static-analysis/writing-new/adding-a-check.rst @@ -0,0 +1,107 @@ +.. _add_a_check: + +Adding a check +============== + +After you've completed a matcher using clang-query, it's time to take it to the next step and turn it into C++ and run it on the whole m-c codebase and see what happens. + +Clang plugins live in `build/clang-plugin <https://searchfox.org/mozilla-central/source/build/clang-plugin>`_ and here we'll cover what is needed to add one. To see how the most recent check was added, you can look at the log for `Checks.inc <https://hg.mozilla.org/mozilla-central/log/tip/build/clang-plugin/Checks.inc>`_ which is one of the necessary files to edit. That's also what we'll be covering next. + +Boilerplate Steps to Add a New Check +------------------------------------ + +First pick a name. Pick something that makes sense without punctuation, in no more than 8 words or so. For this example we'll call it "Missing Else In Enum Comparisons". + +#. Add it alphabetically in build/clang-plugin/Checks.inc, ChecksIncludes.inc, and moz.build +#. ``cd build/clang-plugin && touch MissingElseInEnumComparisons.h MissingElseInEnumComparisons.cpp`` +#. Copy the contents of an existing, simple .h file (e.g. `build/clang-plugin/ScopeChecker.h <https://searchfox.org/mozilla-central/source/build/clang-plugin/ScopeChecker.h>`_) and edit the class name and header guards. +#. Create the following boilerplate for your implementation: + +:: + + /* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + + #include "MissingElseInEnumComparisons.h" + #include "CustomMatchers.h" + + void MissingElseInEnumComparisons::registerMatchers(MatchFinder *AstMatcher) { + + } + + void MissingElseInEnumComparisons::check(const MatchFinder::MatchResult &Result) { + + } + + +Converting your matcher to C++ +------------------------------ +With the boilerplate out of the way, now we can focus on converting the matcher over to C++. Once it's in C++ you'll also be able to take advantage of techniques that will make your matcher easier to read and understand. + +The gist of converting your matcher is to take the following pseudo-code and paste your entire matcher in where 'foo' is; keeping the `.bind("node")` there: + +:: + + AstMatcher->addMatcher( + traverse(TK_IgnoreUnlessSpelledInSource, + foo + .bind("node")), + this); + + +It honest to god is usually that easy. Here's a working example where I pasted in straight from Compiler Explorer: + +:: + + AstMatcher->addMatcher( + traverse(TK_IgnoreUnlessSpelledInSource, + ifStmt(allOf( + has( + binaryOperator( + has( + declRefExpr(hasType(enumDecl().bind("enum"))) + ) + ) + ), + hasElse( + ifStmt(allOf( + unless(hasElse(anything())), + has( + binaryOperator( + has( + declRefExpr(hasType(enumDecl())) + ) + ) + ) + )) + ) + )) + .bind("node")), + this); + + + +If for some reason you're not using the ``IgnoreUnlessSpelledInSource`` Traversal Mode, remove the call to traverse and the corresponding closing paren. (Also, if you're comparing this code to existing source code, know that because this traversal mode is a new clang feature, most historical clang checks do not use it.) + +Wiring up Warnings and Errors +----------------------------- +To get started with a some simple output, just take the boilerplate warning here and stick it in: + +:: + + const auto *MatchedDecl = Result.Nodes.getNodeAs<IfStmt>("node"); + diag(MatchedDecl->getIfLoc(), + "Enum comparisons in an if/else if block without a trailing else.", + DiagnosticIDs::Warning); + + +You'll need to edit two things: + +#. Make sure "node" matches whatever you put in `.bind()` up above. +#. ``getNodeAs<IfStmt>`` needs to be changed to whatever type of element "node" is. Above, we bind "node" to an ifStmt so that's what we need to cast it to. Doing this step incorrectly will cause clang to crash during compilation as if there was some internal compiler error. + + +Running it on Central +---------------------- +After this, the next thing to do is to add ``ac_add_options --enable-clang-plugin`` to your .mozconfig and do a build. Your plugin will be automatically compiled and used across the entire codebase. I suggest using ``./mach build | tee output.txt`` and then ``grep "Enum comparisons" output.txt | cut -d " " -f 3- | sort | uniq``. (The ``cut`` is there to get rid of the timestamp in the line.) diff --git a/docs/code-quality/static-analysis/writing-new/advanced-check-features.rst b/docs/code-quality/static-analysis/writing-new/advanced-check-features.rst new file mode 100644 index 0000000000..e8adcf664f --- /dev/null +++ b/docs/code-quality/static-analysis/writing-new/advanced-check-features.rst @@ -0,0 +1,148 @@ +.. _advanced_check_features: + +Advanced Check Features +======================= + +This page covers additional ways to improve and extend the check you've added to build/clang-plugin. + +Adding Tests +------------ + +No doubt you've seen the tests for existing checks in `build/clang-plugin/tests <https://searchfox.org/mozilla-central/source/build/clang-plugin/tests>`_. Adding tests is straightforward; and your reviewer should insist you do so. Simply copying the existing format of any test and how diagnostics are marked as expected. + +One wrinkle - all clang plugin checks are applied to all tests. We try to write tests so that only one check applies to it. If you write a check that triggers on an existing test, try to fix the existing test slightly so the new check does not trigger on it. + +Using Bind To Output More Useful Information +-------------------------------------------- + +You've probably been wondering what the heck ``.bind()`` is for. You've been seeing it all over the place but never has it actually been explained what it's for and when to use it. + +``.bind()`` is used to give a name to part of the AST discovered through your matcher, so you can use it later. Let's go back to our sample matcher: + +:: + + AstMatcher->addMatcher( + traverse(TK_IgnoreUnlessSpelledInSource, + ifStmt(allOf( + has( + binaryOperator( + has( + declRefExpr(hasType(enumDecl())) + ) + ) + ), + hasElse( + ifStmt(allOf( + unless(hasElse(anything())), + has( + binaryOperator( + has( + declRefExpr(hasType(enumDecl())) + ) + ) + ) + )) + ) + )) + .bind("node")), + this); + +Now the ``.bind("node")`` makes more sense. We're naming the If statement we matched, so we can refer to it later when we call ``Result.Nodes.getNodeAs<IfStmt>("node")``. + +Let's say we want to provide the *type* of the enum in our warning message. There are two enums we end up seeing in our matcher - the enum in the first if statement, and the enum in the second. We're going to arbitrarily pick the first and give it the name ``enumType``: + +:: + + AstMatcher->addMatcher( + traverse(TK_IgnoreUnlessSpelledInSource, + ifStmt(allOf( + has( + binaryOperator( + has( + declRefExpr(hasType(enumDecl().bind("enumType"))) + ) + ) + ), + hasElse( + ifStmt(allOf( + unless(hasElse(anything())), + has( + binaryOperator( + has( + declRefExpr(hasType(enumDecl())) + ) + ) + ) + )) + ) + )) + .bind("node")), + this); + +And in our check() function, we can use it like so: + +:: + + void MissingElseInEnumComparisons::check( + const MatchFinder::MatchResult &Result) { + const auto *MatchedDecl = Result.Nodes.getNodeAs<IfStmt>("node"); + const auto *EnumType = Result.Nodes.getNodeAs<EnumDecl>("enumType"); + + diag(MatchedDecl->getIfLoc(), + "Enum comparisons to %0 in an if/else if block without a trailing else.", + DiagnosticIDs::Warning) << EnumType->getName(); + } + +Repeated matcher calls +-------------------------- + +If you find yourself repeating the same several matchers in several spots, you can turn it into a variable to use. + +:: + + auto isTemporaryLifetimeBoundCall = + cxxMemberCallExpr( + onImplicitObjectArgument(anyOf(has(cxxTemporaryObjectExpr()), + has(materializeTemporaryExpr()))), + callee(functionDecl(isMozTemporaryLifetimeBound()))); + + auto hasTemporaryLifetimeBoundCall = + anyOf(isTemporaryLifetimeBoundCall, + conditionalOperator( + anyOf(hasFalseExpression(isTemporaryLifetimeBoundCall), + hasTrueExpression(isTemporaryLifetimeBoundCall)))); + +The above example is parameter-less, but if you need to supply a parameter that changes, you can turn it into a lambda: + +:: + + auto hasConstCharPtrParam = [](const unsigned int Position) { + return hasParameter( + Position, hasType(hasCanonicalType(pointsTo(asString("const char"))))); + }; + + auto hasParamOfType = [](const unsigned int Position, const char *Name) { + return hasParameter(Position, hasType(asString(Name))); + }; + + auto hasIntegerParam = [](const unsigned int Position) { + return hasParameter(Position, hasType(isInteger())); + }; + + AstMatcher->addMatcher( + callExpr( + hasName("fopen"), + hasConstCharPtrParam(0)) + .bind("funcCall"), + this); + + +Allow-listing existing callsites +-------------------------------- + +While it's not a great situation, you can set up an allow-list of existing callsites if you need to. A simple allow-list is demonstrated in `NoGetPrincipalURI <https://hg.mozilla.org/mozilla-central/rev/fb60b22ee6616521b386d90aec07b03b77905f4e>`_. The `NoNewThreadsChecker <https://hg.mozilla.org/mozilla-central/rev/f400f164b3947b4dd54089a36ea31cca2d72805b>`_ is an example of a more sophisticated way of setting up a larger allow-list. + + +Custom Annotations +------------------ +It's possible to create custom annotations that will be a no-op when compiled, but can be used by a static analysis check. These can be used to annotate special types of sources and sinks (for example). We have some examples of this in-tree presently (such as ``MOZ_CAN_RUN_SCRIPT``) but currently don't have a detailed walkthrough in this documentation of how to set these up and use them. (Patches welcome.) diff --git a/docs/code-quality/static-analysis/writing-new/clang-query.rst b/docs/code-quality/static-analysis/writing-new/clang-query.rst new file mode 100644 index 0000000000..1308a32821 --- /dev/null +++ b/docs/code-quality/static-analysis/writing-new/clang-query.rst @@ -0,0 +1,167 @@ +.. _using_clang_query: + +Using clang-query +================= + +clang-query is a tool that allows you to quickly iterate and develop the difficult part of a matcher. +Once the design of the matcher is completed, it can be transferred to a C++ clang-tidy plugin, `similar +to the ones in mozilla-central <https://searchfox.org/mozilla-central/source/build/clang-plugin>`_. + +Recommended Boilerplate +----------------------- + +:: + + set traversal IgnoreUnlessSpelledInSource + set bind-root true + # ^ true unless you use any .bind("foo") commands + set print-matcher true + enable output dump + + +clang-query Options +------------------- + +set traversal +~~~~~~~~~~~~~ + +`Traversal mode <https://clang.llvm.org/docs/LibASTMatchersReference.html#traverse-mode>`_ specifies how the AST Matcher will traverse the nodes in the Abstract Syntax Tree. There are two values: + +AsIs + This mode notes all the nodes in the AST, even if they are not explicitly spelled out in the source. This will include nodes you have never seen and probably don't immediately understand, for example ``ExprWithCleanups`` and ``MaterializeTemporaryExpr``. In this mode, it is necessary to write matchers that expliticly match or otherwise traverse these potentially unexpected nodes. + +IgnoreUnlessSpelledInSource + This mode skips over 'implicit' nodes that are created as a result of implicit casts or other usually-low-level language details. This is typically much more user-friendly. **Typically you would want to use** ``set traversal IgnoreUnlessSpelledInSource``. + +More examples are available `in the documentation <https://clang.llvm.org/docs/LibASTMatchersReference.html#traverse-mode>`_, but here is a simple example: + +:: + + B func1() { + return 42; + } + + /* + AST Dump in 'Asis' mode for C++17/C++20 dialect: + + FunctionDecl + `-CompoundStmt + `-ReturnStmt + `-ImplicitCastExpr + `-CXXConstructExpr + `-IntegerLiteral 'int' 42 + + AST Dump in 'IgnoreUnlessSpelledInSource' mode for all dialects: + + FunctionDecl + `-CompoundStmt + `-ReturnStmt + `-IntegerLiteral 'int' 42 + */ + + +set bind-root +~~~~~~~~~~~~~ + +If you are matching objects and assigning them names for later use, this option may be relevant. If you are debugging a single matcher and not using any ``.bind()``, it is irrelevant. + +Consider the output of ``match functionDecl().bind("x")``: + +:: + + clang-query> match functionDecl().bind("x") + + Match #1: + + testfile.cpp:1:1: note: "root" binds here + int addTwo(int num) + ^~~~~~~~~~~~~~~~~~~ + testfile.cpp:1:1: note: "x" binds here + int addTwo(int num) + ^~~~~~~~~~~~~~~~~~~ + + Match #2: + + testfile.cpp:6:1: note: "root" binds here + int main(int, char**) + ^~~~~~~~~~~~~~~~~~~~~ + testfile.cpp:6:1: note: "x" binds here + int main(int, char**) + ^~~~~~~~~~~~~~~~~~~~~ + 2 matches. + + +clang-query automatically binds ``root`` to the match, but we also bound the name ``x`` to that match. The ``root`` is redundant. If you ``set bind-root false`` then the output is less noisy: + +:: + + clang-query> set bind-root false + clang-query> m functionDecl().bind("x") + + Match #1: + + testfile.cpp:1:1: note: "x" binds here + int addtwo(int num) + ^~~~~~~~~~~~~~~~~~~ + + Match #2: + + testfile.cpp:6:1: note: "x" binds here + int main(int, char**) + ^~~~~~~~~~~~~~~~~~~~~ + 2 matches. + + +set print-matcher +~~~~~~~~~~~~~~~~~ + +``set print-matcher true`` will print a header line of the form 'Matcher: <foo>' where foo is the matcher you have written. It is helpful when debugging multiple matchers at the same time, and no inconvience otherwise. + +enable/disable/set output <foo> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +These commands will control the type of output you get from clang-query. The options are: + +``print`` + Shows you the C++ form of the node you are matching. This is typically not useful. + +``diag`` + Shows you the individual node you are matching. + +``dump`` (alias: ``detailed-ast``) + Shows you the node you are matching and the entire subtree for the node + +By default, you get ``diag`` output. You can change the output by choosing ``set output``. You can *add* output by using ``enable output``. You can *disable* output using ``disable output`` but this is typically not needed. + +So if you want to get all three output formats you can do: + +:: + + # diag output happens automatically because you did not override with 'set' + enable output print + enable output dump + + +Patches +------- + +This section tracks some patches; they are currently not used, but we may want them in the future. + +- Functionality: + + - `traverse() operator available to clang-query <https://reviews.llvm.org/D80654>`_ + - `srclog output <https://reviews.llvm.org/D93325>`_ + - `allow anyOf() to be empty <https://reviews.llvm.org/D94126>`_ + - breakpoints + - debug + - profile + +- Matcher Changes: + + - `binaryOperation() matcher <https://reviews.llvm.org/D94129>`_ + +- Plumbing: + + - `mapAnyOf() <https://reviews.llvm.org/D94127>`_ (`Example of usage <https://reviews.llvm.org/D94131>`_) + - `Make cxxOperatorCallExpr matchers API-compatible with n-ary operators <https://reviews.llvm.org/D94128>`_ + - `CXXRewrittenBinaryOperator <https://reviews.llvm.org/D94130>`_ diff --git a/docs/code-quality/static-analysis/writing-new/documentation-expanded.png b/docs/code-quality/static-analysis/writing-new/documentation-expanded.png Binary files differnew file mode 100644 index 0000000000..82f12a516d --- /dev/null +++ b/docs/code-quality/static-analysis/writing-new/documentation-expanded.png diff --git a/docs/code-quality/static-analysis/writing-new/index.rst b/docs/code-quality/static-analysis/writing-new/index.rst new file mode 100644 index 0000000000..9107fb1b59 --- /dev/null +++ b/docs/code-quality/static-analysis/writing-new/index.rst @@ -0,0 +1,14 @@ +Writing New Firefox-Specific Static Analysis Checks +=================================================== + +This section is intended to help Mozilla engineers either casually play with writing a static analysis check +or seriously develop one we can land and run internally. While being written for internal consumption, it's broadly applicable outside Mozilla. + +.. toctree:: + :maxdepth: 2 + + clang-query.rst + writing-matchers.rst + matcher-cookbook.rst + adding-a-check.rst + advanced-check-features.rst diff --git a/docs/code-quality/static-analysis/writing-new/matcher-cookbook.rst b/docs/code-quality/static-analysis/writing-new/matcher-cookbook.rst new file mode 100644 index 0000000000..9eb0d96c43 --- /dev/null +++ b/docs/code-quality/static-analysis/writing-new/matcher-cookbook.rst @@ -0,0 +1,23 @@ +.. _matcher_cookbook: + +Matcher Cookbook +================= + +This page is designed to be a selection of common ingredients to a more complicated matcher. + +.. list-table:: + :widths: 35 65 + :header-rows: 1 + :class: matcher-cookbook + + * - Desired Outcome + - Syntax + * - Ignore header files + + *If you have an #include in your example code, your matcher may match things in the header files.* + - Add **isExpansionInMainFile()** to the matcher. e.g. + + ``m functionDecl(isExpansionInMainFile())`` + + +*More coming* diff --git a/docs/code-quality/static-analysis/writing-new/narrowing-matcher.png b/docs/code-quality/static-analysis/writing-new/narrowing-matcher.png Binary files differnew file mode 100644 index 0000000000..52c82791d3 --- /dev/null +++ b/docs/code-quality/static-analysis/writing-new/narrowing-matcher.png diff --git a/docs/code-quality/static-analysis/writing-new/narrowing-matcher.xcf b/docs/code-quality/static-analysis/writing-new/narrowing-matcher.xcf Binary files differnew file mode 100644 index 0000000000..0102f79e76 --- /dev/null +++ b/docs/code-quality/static-analysis/writing-new/narrowing-matcher.xcf diff --git a/docs/code-quality/static-analysis/writing-new/writing-matchers.rst b/docs/code-quality/static-analysis/writing-new/writing-matchers.rst new file mode 100644 index 0000000000..5b693f5f27 --- /dev/null +++ b/docs/code-quality/static-analysis/writing-new/writing-matchers.rst @@ -0,0 +1,199 @@ +.. _writing_matchers: + +Writing Matchers +================ + +On this page we will give some information about what a matcher is, and then provide an example of developing a simple match iteratively. + +Types of Matchers +----------------- + +There are three types of matches: Node, Narrowing, and Traversal. There isn't always a clear separation or distinction between them, so treat this explanation as illustrative rather than definitive. Here is the documentation on matchers: `https://clang.llvm.org/docs/LibASTMatchersReference.html <https://clang.llvm.org/docs/LibASTMatchersReference.html>`_ + +On that page it is not obvious, so we want to note, **cicking on the name of a matcher expands help about that matcher.** Example: + +.. image:: documentation-expanded.png + +Node Matchers +~~~~~~~~~~~~~ + +Node matchers can be thought of as 'Nouns'. They specify a **type** of node you want to match, that is, a particular *thing*. A function, a binary operation, a variable, a type. + +A full list of `node matchers are listed in the documentation <https://clang.llvm.org/docs/LibASTMatchersReference.html#node-matchers>`_. Some common ones are ``functionDecl()``, ``binaryOperator()``, and ``stmt()``. + +Narrowing Matchers +~~~~~~~~~~~~~~~~~~ + +Narrowing matchers can be thought of as 'Adjectives'. They narrow, or describe, a node, and therefore must be applied to a Node Matcher. For instance a node matcher may be a ``functionDecl``, and the narrowing matcher applied to it may be ``parameterCountIs``. + +The `table in the documentation <https://clang.llvm.org/docs/LibASTMatchersReference.html#narrowing-matchers>`_ lists all the narrowing matchers, which they apply to and how to use them. Here is how to read the table: + +.. image:: narrowing-matcher.png + +And some examples: + +:: + + m functionDecl(parameterCountIs(1)) + m functionDecl(anyOf(isDefinition(), isVariadic())) + + +As you can see **only one Narrowing Matcher is allowed** and it goes inside the parens of the Node Matcher. In the first example, the matcher is ``parameterCountIs``, in the second it is ``anyOf``. + +In the second, we use the singular ``anyOf`` matcher to match any of multiple other Narrowing Matchers: ``isDefinition`` or ``isVariadic``. The other two common combining narrowing matchers are ``allOf()`` and ``unless()``. + +If you *need* to specify a narrowing matcher (because it's a required argument to some other matcher), you can use the ``anything()`` narrowing matcher to have a no-op narrowing matcher. + +Traversal Matchers +~~~~~~~~~~~~~~~~~~ + +Traversal Matchers *also* can be thought of as adjectives - at least most of them. They also describe a specific node, but the difference from a narrowing matcher is that the scope of the description is broader than the individual node. A narrowing matcher says something about the node in isolation (e.g. the number of arguments it has) while a traversal matcher says something about the node's contents or place in the program. + +Again, the `the documentation <https://clang.llvm.org/docs/LibASTMatchersReference.html#traversal-matchers>`_ is the best place to explore and understand these, but here is a simple example for the traversal matcher ``hasArraySize()``: + +:: + + Given: + class MyClass { }; + MyClass *p1 = new MyClass[10]; + + + cxxNewExpr() + matches the expression 'new MyClass[10]'. + + cxxNewExpr(hasArraySize(integerLiteral(equals(9)))) + does not match anything + + cxxNewExpr(hasArraySize(integerLiteral(equals(10)))) + matches the expression 'new MyClass[10]'. + + + +Example of Iterative Matcher Development +---------------------------------------- + +When developing matchers, it will be much easier if you do the following: + +1. Write out the code you want to match. Write it out in as many different ways as you can. Examples: For some value in the code use a variable, a constant and a function that returns a value. Put the code you want to match inside of a function, inside of a conditional, inside of a function call, and inside of an inline function definition. +2. Write out the code you *don't* want to match, but looks like code you do. Write out benign function calls, benign assignments, etc. +3. Iterate on your matcher and treat it as _code_ you're writing. Indent it, copy it somewhere in case your browser crashes, even stick it in a tiny temporary version-controlled file. + +As an example of the above, below is a sample iterative development process of a more complicated matcher. + + **Goal**: Match function calls where one of the parameters is an assignment expression with an integer literal, but the function parameter has a default value in the function definition. + +:: + + int add1(int a, int b) { return a + b; } + int add2(int c, int d = 8) { return c + d; } + + int main() { + int x, y, z; + + add1(x, y); // <- No match, no assignment + add1(3 + 4, y); // <- No match, no assignment + add1(z = x, y); // <- No match, assignment, but not an integer literal + add1(z = 2, y); // <- No match, assignment, integer literal, but function parameter lacks default value + add2(3, z = 2); // <- Match + } + + +Here is the iterative development process: + +:: + + //------------------------------------- + // Step 1: Find all the function calls + m callExpr() + // Matches all calls, as expected. + + //------------------------------------- + // Step 2: Start refining based on the arguments to the call + m callExpr(forEachArgumentWithParam())) + // Error: forEachArgumentWithParam expects two parameters + + //------------------------------------- + // Step 3: Figure out the syntax to matching all the calls with this new operator + m callExpr( + forEachArgumentWithParam( + anything(), + anything() + ) + ) + // Matches all calls, as expected + + //------------------------------------- + // Step 4: Find the calls with a binary operator of any kind + m callExpr( + forEachArgumentWithParam( + binaryOperator(), + anything() + ) + ) + // Does not match the first call, but matches the others + + //------------------------------------- + // Step 5: Limit the binary operator to assignments + m callExpr( + forEachArgumentWithParam( + binaryOperator(isAssignmentOperator()), + anything() + ) + ) + // Now matches the final three calls + + //------------------------------------- + // Step 6: Starting to refine matching the right-hand of the assignment + m callExpr( + forEachArgumentWithParam( + binaryOperator( + allOf( + isAssignmentOperator(), + hasRHS() + )), + anything() + ) + ) + // Error, hasRHS expects a parameter + + //------------------------------------- + // Step 7: + m callExpr( + forEachArgumentWithParam( + binaryOperator( + allOf( + isAssignmentOperator(), + hasRHS(anything()) + )), + anything() + ) + ) + // Okay, back to matching the final three calls + + //------------------------------------- + // Step 8: Refine to just integer literals + m callExpr( + forEachArgumentWithParam( + binaryOperator( + allOf( + isAssignmentOperator(), + hasRHS(integerLiteral()) + )), + anything() + ) + ) + // Now we match the final two calls + + //------------------------------------- + // Step 9: Apply a restriction to the parameter definition + m callExpr( + forEachArgumentWithParam( + binaryOperator( + allOf( + isAssignmentOperator(), + hasRHS(integerLiteral()) + )), + hasDefaultArgument() + ) + ) + // Now we match the final call |
