diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 026714abb23..83b30ed587f 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -10,7 +10,7 @@ # @BrucePay @JamesWTruher # Area: Security -# @TravisEz13 @PaulHigin @chunqingchen +# @TravisEz13 @PaulHigin # Area: Documentation # @joeyaiello @TravisEz13 diff --git a/.github/CONTRIBUTING.md b/.github/CONTRIBUTING.md index 4aaef236d39..a7491ea8bd9 100644 --- a/.github/CONTRIBUTING.md +++ b/.github/CONTRIBUTING.md @@ -111,15 +111,23 @@ Additional references: ![Github-PR-dev.png](Images/Github-PR-dev.png) +* It's recommended to avoid a PR with too many changes. + A large PR not only stretches the review time, but also makes it much harder to spot issues. + In such case, it's better to split the PR to multiple smaller ones. + For large features, try to approach it in an incremental way, so that each PR won't be too big. * If you're contributing in a way that changes the user or developer experience, you are expected to document those changes. See [Contributing to documentation related to PowerShell](#contributing-to-documentation-related-to-powershell). * Add a meaningful title of the PR describing what change you want to check in. - Don't simply put: "Fixes issue #5". - A better example is: "Add Ensure parameter to New-Item cmdlet", with "Fixes #5" in the PR's body. + Don't simply put: "Fix issue #5". + Also don't directly use the issue title as the PR title. + An issue title is to briefly describe what is wrong, while a PR title is to briefly describe what is changed. + A better example is: "Add Ensure parameter to New-Item cmdlet", with "Fix #5" in the PR's body. * When you create a pull request, - including a summary of what's included in your changes and - if the changes are related to an existing GitHub issue, - please reference the issue in pull request description (e.g. ```Closes #11```). + including a summary about your changes in the PR description. + The description is used to create change logs, + so try to have the first sentence explain the benefit to end users. + If the changes are related to an existing GitHub issue, + please reference the issue in PR description (e.g. ```Fix #11```). See [this][closing-via-message] for more details. * If the change warrants a note in the [changelog](../CHANGELOG.MD) either update the changelog in your pull request or diff --git a/.spelling b/.spelling index 40d7995fdd6..11edaadb281 100644 --- a/.spelling +++ b/.spelling @@ -955,3 +955,9 @@ microsoft.com - ./tools/install-powershell.readme.md includeide sed + - docs/dev-process/coding-guidelines.md +interop +PaulHigin +SMEs +TravisEz13 +uppercase diff --git a/docs/dev-process/coding-guidelines.md b/docs/dev-process/coding-guidelines.md index 74e25cbf140..a6ba927d474 100644 --- a/docs/dev-process/coding-guidelines.md +++ b/docs/dev-process/coding-guidelines.md @@ -1,92 +1,204 @@ - -# C# Coding Style +# C# Coding Guidelines ## Coding Conventions As a general rule, our coding convention is to follow the style of the surrounding code. -Avoid reformatting any code when submitting a PR as it obscures the functional changes of your change. -We run the [.NET code formatter tool](https://github.com/dotnet/codeformatter) regularly help keep consistent formatting. +So if a file happens to differ in style from conventions defined here +(e.g. private members are named `m_member` rather than `_member`), +the existing style in that file takes precedence. + +When making changes, you may find some existing code goes against the conventions defined here. +In such cases, please avoid reformatting any existing code when submitting a PR as it obscures the functional changes of the PR. +A separate PR should be submitted for style-only changes. +We also run the [.NET code formatter tool](https://github.com/dotnet/codeformatter) regularly to keep consistent formatting. + +### Naming Conventions + +* Use meaningful, descriptive words for names. + For method names, it's encouraged to use `VerbObject` pair such as **`LoadModule`**. + +* Use `_camelCase` to name internal and private fields and use `readonly` where possible. + Prefix instance fields with `_`, static fields with `s_` and thread static fields with `t_`. + When used on static fields, `readonly` should come after `static` (i.e. `static readonly` not `readonly static`). + +* Use `camelCase` to name non-constant local variables. -A basic rule of formatting is to use "Visual Studio defaults". -Here are some general guidelines +* Use `PascalCase` to name constant local variables and fields. + The only exception is for interop code where the constant should exactly match the name and value of the code you are calling via interop (e.g. `const int ERROR_SUCCESS = 0`). -* No tabs, indent 4 spaces. -* Braces usually go on their own line, +* Use `PascalCase` to name types and all other type members. + +### Layout Conventions + +* Use four spaces of indentation (no tabs). + +* Avoid more than one blank empty line at any time. + +* Avoid trailing spaces at the end of a line. + +* Braces usually go on their own lines, with the exception of single line statements that are properly indented. -* Use `_camelCase` for instance fields, - use `readonly` where possible. + +* Namespace imports should be specified at the top of the file, + outside of `namespace` declarations. + +* Fields should be specified at the top within type declarations. + For those that serve as backing fields for properties, + they should be specified next to the corresponding properties. + +* Preprocessor directives like `#if` and `#endif` should be placed at the beginning of a line, + without any leading spaces. + +* File encoding should be `ASCII`. + All `BOM` encodings should be avoided. + Tests that need a `BOM` encoding file should generate the file on the fly. + +### Member Conventions + * Use of `this` is neither encouraged nor discouraged. -* Avoid more than one blank empty line. -* Public members should use [doc comments](https://msdn.microsoft.com/en-us/library/b2s063f7.aspx), - internal members may use doc comments but it is not encouraged. + +* Use `nameof()` instead of `""` whenever possible and relevant. + The motivation is to easily and more accurately find references. + +* Always specify the visibility, even if it's the default (i.e. `private string _foo` not `string _foo`). + Visibility should be the first modifier (i.e. `public abstract` not `abstract public`). + +* Make members private where possible. + Avoid declaring public members unless it's absolutely necessary. + * Public members in a namespace that ends with `Internal`, for example `System.Management.Automation.Internal` are not considered a supported public API. Such members are necessarily public as implementation details in code shared between C# and PowerShell script, or must be available publicly by generated code. -* File encoding should be ASCII (preferred) - or UTF8 (with BOM) if absolutely necessary. -## Preprocessor defines +### Commenting Conventions -There are 3 primary preprocessor macros we define during builds: +* Place the comment on a separate line, not at the end of a line of code. -* DEBUG - guard code that should not be included in release builds -* CORECLR - guard code that differs between Full CLR and CoreCLR -* UNIX - guard code that is specific to Unix (Linux and macOS) +* Begin comment text with an uppercase letter. + It's recommended to end comment text with a period but not required. -Any other preprocessor defines found in the source are used for one-off custom builds, -typically to help debug specific scenarios. +* Add comments where the code is not trivial or could be confusing. -### Runtimes +* Add comments where a reviewer needs help to understand the code. -The PowerShell repo is used to build PowerShell targeting CoreCLR as well as CLR 4.5. +* Update/remove existing comments when you are changing the corresponding code. -Code under !CORECLR must build against CLR 4.5. -We will not accept changes that require a later version of the full CLR. -In extremely rare cases, we may use reflection to use an API in a later version of the CLR, -but the feature must robustly handle running with CLR 4.5. +* Make sure the added/updated comments are meaningful, accurate and easy to understand. -We may reject code under !CORECLR without explanation because -we do not support installation or testing of such code in this repo. -All new features should support CoreCLR. +* Public members must use [doc comments](https://msdn.microsoft.com/en-us/library/b2s063f7.aspx). + Internal and private members may use doc comments but it is not required. -## Performance considerations +## Performance Considerations PowerShell has a lot of performance sensitive code as well as a lot of inefficient code. -We have some guidelines that we typically apply widely even in less important code -because code and patterns are copied we want certain inefficient code to stay out of the performance critical code. +We have some guidelines that we typically apply widely even in less important code because code and patterns are copied, +and we want certain inefficient code to stay out of the performance critical code. Some general guidelines: -* Avoid LINQ - it can create lots of avoidable garbage -* Prefer `for` and `foreach`, - with a slight preference towards `for` when you're uncertain if `foreach` allocates an iterator. +* Avoid LINQ - it can create lots of avoidable garbage. + Instead, iterate through a collection directly using `for` or `foreach` loop. + +* Between `for` and `foreach`, + `for` is slightly preferred when you're uncertain if `foreach` allocates an iterator. + * Avoid `params` arrays, prefer adding overloads with 1, 2, 3, and maybe more parameters. + * Be aware of APIs such as `String.Split(params char[])` that do not provide overloads to avoid array allocation. - When calling such APIs, reuse a static array when possible. + When calling such APIs, reuse a static array when possible (e.g. `Utils.Separators.Colon`). + +* Avoid creating empty arrays. + Instead, reuse the static ones via `Utils.EmptyArray`. + * Avoid unnecessary memory allocation in a loop. Move the memory allocation outside the loop if possible. -## Portable code +* Avoid gratuitous exceptions as much as possible. + Exception handling can be expensive due to cache misses and page faults when accessing the handling code and data. + Finding and designing away exception-heavy code can result in a decent performance win. + For example, you should stay away from things like using exceptions for control flow. + +* Avoid `if (obj is Example) { example = (Example)obj }` when casting an object to a type. + Instead, use `var example = obj as Example` or the C# 7 syntax `if (obj is Example example) {...}` as appropriate. + In this way you can avoid converting to the type twice. + +* Use `dict.TryGetValue` instead of `dict.Contains` and `dict[..]` when retrieving value from a `Dictionary`. + In this way you can avoid hashing the key twice. + +* It's OK to use the `+` operator to concatenate one-off short strings. + But when dealing with strings in loops or large amounts of text, + use a `StringBuilder` object. + +## Security Considerations + +Security is an important aspect of PowerShell and we need to be very careful about changes that may introduce security risks, +such as code injection caused by the lack of input validation, +privilege escalation due to the misuse of impersonation, +or data privacy breach with a plain text password. + +Reviewers of a PR should be sensitive to changes that may affect security. +Some security related keywords may serve as good indicators, +such as `password`, `crypto`, `encryption`, `decryption`, `certificate`, `authenticate`, `ssl/tls` and `protected data`. + +When facing a PR with such changes, +the reviewers should request a designated security Subject Matter Expert (SME) to review the PR. +Currently, @PaulHigin and @TravisEz13 are our security SMEs. +See [CODEOWNERS](../../.github/CODEOWNERS) for more information about the area experts. + +## Best Practices -The PowerShell code base started on Windows and depends on many Win32 APIs through P/Invoke. -Going forward, we try to depend on CoreCLR to handle platform differences, -so avoid adding new P/Invoke calls where a suitable alternative exists in .NET. +* Avoid hard-coding anything unless it's absolutely necessary. + +* Avoid a method that is too long and complex. + In such case, separate it to multiple methods or even a nested class as you see fit. + +* Use `using` statement instead of `try/finally` if the only code in the `finally` block is to call the `Dispose` method. + +* Use of object initializers (e.g. `new Example { Name = "Name", ID = 1 }`) is encouraged for better readability, + but not required. + +* Stick to the `DRY` principle -- Don't Repeat Yourself. + * Wrap the commonly used code in methods, + or even put it in a utility class if that makes sense, + so that the same code can be reused (e.g. `StringToBase64Converter.Base64ToString(string)`). + * Check if the code for the same purpose already exists in the code base before inventing your own wheel. + * Avoid repeating literal strings in code. Instead, use `const` variable to hold the string. + * Resource strings used for errors or UI should be put in resource files (`.resx`) so that they can be localized later. + +* Use of new C# language syntax is encouraged. + But avoid refactoring any existing code using new language syntax when submitting a PR + as it obscures the functional changes of the PR. + A separate PR should be submitted for such refactoring without any functional changes. + +## Portable Code + +There are 3 primary preprocessor macros we use during builds: + +* `DEBUG` - guard code that should not be included in release builds +* `CORECLR` - guard code that differs between Full CLR and CoreCLR +* `UNIX` - guard code that is specific to Unix (Linux and macOS) + +Any other preprocessor defines found in the source are used for one-off custom builds, +typically to help debug specific scenarios. -Try to minimize the use of `#if UNIX` and `#if CORECLR`. -When absolutely necessary, avoid duplicating more code than necessary, -and instead prefer introducing helper functions to minimize the platform differences. +Here are some general guidelines for writing portable code: -When adding platform dependent code, prefer preprocessor directives -over runtime checks. +* We are in the process of cleaning up Full CLR specific code (code enclosed in `!CORECLR`), + so do not use `CORECLR` or `!CORECLR` in new code. + PowerShell Core targets .NET Core only and all new changes should support .NET Core only. -We produce a single binary for all UNIX variants, -so runtime checks are currently necessary for some platform differences, e.g. macOS and Linux. +* The PowerShell code base started on Windows and depends on many Win32 APIs through P/Invoke. + Going forward, we try to depend on .NET Core to handle platform differences, + so avoid adding new P/Invoke calls where a suitable alternative exists in .NET Core. -## Code comments +* Try to minimize the use of `#if UNIX`. + When absolutely necessary, avoid duplicating more code than necessary, + and instead prefer introducing helper functions to minimize the platform differences. -It's strongly encouraged to add comments when you are making changes to the code and tests, -especially when the changes are not trivial or may raise confusion. -Make sure the added comments are accurate and easy to understand. -Good code comments would greatly improve readability of the code, and make it much more maintainable. +* When adding platform dependent code (`Windows` vs. `UNIX`), prefer preprocessor directives over runtime checks. + However, runtime checks are acceptable if it would greatly improve readability + without causing performance concerns in performance-sensitive code. +* We produce a single binary for all UNIX variants, + so runtime checks are currently necessary for some of them (e.g. macOS vs. Linux).