-
Notifications
You must be signed in to change notification settings - Fork 8
Add style guideline chapter #27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add style guideline chapter #27
Conversation
src/process/style-guideline.rst
Outdated
|
||
* ``draft`` | ||
* ``approved`` | ||
* ``deprecated`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion from @rcseacord to:
- remove
deprecated
as likely tool vendors will not need to issue warnings for no longer in force guidelines - add instead a
retired
category on which to make the guideline no longer in force
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
n.b. the model for tool vendors will often be along the lines of unconditionally supporting every warning, with configuration left flexible to suit the user's prokect-level recategorization plan; so a category for rules that are no longer wanted by default is useful because for project-specific reasons users may want to re-enable those rules.
As a rule proceeds through its lifecycle, a tool vendor will not usually mirror that lifecycle in the corresponding warning: once any user is using a warning to enforce a rule that is Approved in their version of the document, that warning will generally be retained permanently because users do not change standard or language version rapidly, if at all, and usually find themselves bound to both a specific language revision and a specific Guidelines revision, independently of whether they can upgrade their tooling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do agree with Alex' analysis of tool vendor behavior and agree with the change to "retired".
To me "deprecated" means that something in that category is subject to removal in future. Given we have no plans to ever remove a guideline, it may be misleading whereas I could expect a rule to sit in "retired" forever.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, I'll take up @rcseacord's suggestion then and incorporate it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in commit 53c9973c643168f308300cffd5daf803f2d7f13b
or in order to make a single guideline more granular and replace it with | ||
more than one guideline. | ||
|
||
For more, see :ref:`Guideline Lifecycle`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo @PLeVasseur -- flesh this in a little before next we meet to better solicit feedback.
|
||
These guidelines are not in force and **MUST** not be applied. | ||
|
||
``release`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding feedback here to ruminate on:
- use release ranges?
- earliest release may need only be as early as the first safety-certified Rust compiler
- committing to a stable release plan which is not every release for now likely fine due to slower nature of compiler adoption in safety-critical
- nature of using digital tools could allow for things like
- having a flag to
./make.py
which would compile out certain guidelines or having a toggle within the generated docs - but -- should we do this? experience with MISRA seems to indicate users may not know the version and thus having a larger blanket approach may be best and not version
- having a flag to
- handling compiler bugs usually not handled in guidelines directly, but supplementary documents such as the MISRA Compliance document for that procedure for users
- Cert rather than versioning guidelines versions copies of the entire standard, does that work here?
|
||
These guidelines are not in force and **MUST** not be applied. | ||
|
||
``release`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed having folks reach out to safety-certified Rust toolchain vendors about their roadmap so we can use this for guidance:
- HighTec - @pellico
- Ferrous Systems - @AlexCeleste
- AdaCore - @AlexCeleste
:: | ||
|
||
.. guideline:: Avoid Implicit Integer Wrapping | ||
:id: gui_xztNdXA2oFNB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestions on IDs from @rcseacord (some overlap with part of #19)
- something human-readable is nice
- even if wanting to stick with
gui_<alphanumeric-string>
it may be nice to help with recalling where the guideline fits by doing something likegui_typ_<alphanumeric-string>
so that users can more easily know this is in the chapter on types and traits for example - having identifiers like this will help with web searchability (yay)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the addition of something in the style of C++ section tags, [types.intwrap]
would be human readable, and wouldn't suffer from renumbering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this be covered if we had some defined things under tags like this? Such that we could have a tag underneath each guideline which denoted where it fit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed this would cover the contents of my suggestion. However, if we are trying to find a human readable identifier for the guidelines typically I would not amalgamate the tags of something to refer to it. "You're looking for a guideline on your left near the 'integral types' and 'Undefined behavior', but if you see the 'traits' you've gone too far" :)
**MUST** be generated using the ``generate-guideline-templates.py`` script to ensure | ||
compliance. | ||
|
||
``category`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo @PLeVasseur -- update this section with guidance that a contributor need not worry about this when initially submitting and we will discuss the right category
.
Can help reduce friction of contributions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or perhaps guidance to strawman as "advisory" if unsure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in commit 6d25a17a10dfb06ac126cf21feb9f1f34fcadcf0
src/process/style-guideline.rst
Outdated
``deprecated`` | ||
^^^^^^^^^^^^^^ | ||
|
||
These guidelines are not in force and **MUST** not be applied. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in practice this shouldn't be a MUST, users will continue to use deprecated guidelines for a variety of reasons. I would suggest mirroring the language for Disapplied status here and having no requirement rather than a negative requirement (i.e. all Deprecated are implicitly Disapplied regardless of recorded category; probably correspondingly that all Draft are implicitly Advisory regardless of recorded category)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, thanks for providing the background.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is addressed by 1a447262fbfc9dd573abfc4e7112869269ab863e, feel free to let me know.
src/process/style-guideline.rst
Outdated
``disapplied`` | ||
^^^^^^^^^^^^^^ | ||
|
||
These are guidelines for which compliance **SHOULD NOT** be required. No enforcement is expected, and any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think SHOULD NOT is too strong here, this is a non-requirement rather than a negative requirement. In the original this was just "is not required". (the MAY below OTOH is exactly correct)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, will revise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is addressed by 1a447262fbfc9dd573abfc4e7112869269ab863e, feel free to let me know.
src/process/style-guideline.rst
Outdated
``tags`` | ||
-------- | ||
|
||
The ``tags`` are largely descriptive, not proscriptive means of finding commonality between |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo "prescriptive" (assuming tags aren't negative :P)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/process/style-guideline.rst
Outdated
|
||
* ``draft`` | ||
* ``approved`` | ||
* ``deprecated`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do agree with Alex' analysis of tool vendor behavior and agree with the change to "retired".
To me "deprecated" means that something in that category is subject to removal in future. Given we have no plans to ever remove a guideline, it may be misleading whereas I could expect a rule to sit in "retired" forever.
:: | ||
|
||
.. guideline:: Avoid Implicit Integer Wrapping | ||
:id: gui_xztNdXA2oFNB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the addition of something in the style of C++ section tags, [types.intwrap]
would be human readable, and wouldn't suffer from renumbering.
**MUST** be generated using the ``generate-guideline-templates.py`` script to ensure | ||
compliance. | ||
|
||
``category`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or perhaps guidance to strawman as "advisory" if unsure.
|
||
Code claimed to be in compliance with this document **MUST** follow every guideline marked as ``mandatory``. | ||
|
||
*TODO(pete.levasseur): Add more tips on when this is a good choice for a guideline.* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some suggestions as a starting point for these tips
*TODO(pete.levasseur): Add more tips on when this is a good choice for a guideline.* | |
Selecting this category is appropriate when any non-compliance of the guideline would result in undefined behavior |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay -- let's bring these in to discuss in the meeting next week. Thanks for the thought starters.
|
||
An organization or project **MAY** choose to recategorize any ``required`` guideline to ``mandatory``. | ||
|
||
*TODO(pete.levasseur): Add more tips on when this is a good choice for a guideline.* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*TODO(pete.levasseur): Add more tips on when this is a good choice for a guideline.* | |
Just some suggestions as a starting point for these tips | |
```suggestion | |
Selecting this category is appropriate when many instances of non-compliance with the guideline would result in undefined behavior or would result in a program state the developer was not expecting. |
An organization or project **MAY** choose to recategorize any ``disapplied`` guideline as ``mandatory`` | ||
or ``required``, or as ``advisory``. | ||
|
||
*TODO(pete.levasseur): Add more tips on when this is a good choice for a guideline.* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*TODO(pete.levasseur): Add more tips on when this is a good choice for a guideline.* | |
Selecting this category is appropriate when instances of non-compliance with the guideline would possibly result in unsafe code or could cause developer confusion. There should be a sound argument as to why non-compliance with a guideline in category would be generally useful. |
An organization or project **MAY** choose to recategorize any ``disapplied`` guideline as ``mandatory`` | ||
or ``required``, or as ``advisory``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The more wording we can just cite from an industry document like MISRA Compliance the better in my opinion. Safety critical projects, at least in the automotive space, are likely already familiar with terms and processes contained within.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, definitely agree. Tried my best to align with MISRA C 2025's definitions. Could you help me understand if there's some action you're suggesting here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking a citation to the compliance document for more details on how recategorization works. But in general I think we can shelf my comment or resolve it, and focus on the action plan in regards to Alex's comment.
Guideline Content **MAY** contain a section titled *Exception* followed by text that that describes | ||
situations in which the guideline does not apply. The use of exceptions permits the description of | ||
some guidelines to be simplified. It is important to note that an exception is a situation in which | ||
a guideline does not apply. Code that complies with a guideline by virtue of an exception does not | ||
require a deviation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a distinction should be made between "does not apply" and "is not enforced" or similar wordings. The guideline on implicit integer wrapping does not apply to this example,
let sum: f32 = f + 10.0;
because the guideline does not apply to floating point arithmetic. No exception could be written for floating point because that is outside the guideline's scope already. For the sake of example let's say we wanted to add an exception to the implicit integer wrapping guidline for adding the integer literal 1
unchecked,
let sum = i + 1;
The rule does apply here as this is unchecked integer arithmetic prone to wraparound, but there is an explicitly provided exception which makes this specific situation OK (because we said so with some good reason). I think an exception should be categorically different than us clarifying in the description of the guideline, "This guideline only applies to integer arithmetic", which sets the scope of the rule's applicability.
Guideline Content **MAY** contain a section titled *Exception* followed by text that that describes | |
situations in which the guideline does not apply. The use of exceptions permits the description of | |
some guidelines to be simplified. It is important to note that an exception is a situation in which | |
a guideline does not apply. Code that complies with a guideline by virtue of an exception does not | |
require a deviation. | |
Guideline Content **MAY** contain a section titled *Exception* followed by text that that describes specific situations where the guideline is not enforced, but would otherwise apply. The use of | |
exceptions permits the description of some guidelines to be simplified. It is important to note that | |
an exception is a situation in which compliance to the guideline is not expected. Code that complies | |
with a guideline by virtue of an exception does not require a deviation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you help me understand how this changes the meaning? I've tried to align closely with MISRA C 2025's definitions.
In general, I agree that
a distinction should be made between "does not apply" and "is not enforced" or similar wordings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In short I see the normative description (amplification) setting the "scope" of what does apply and what does not apply, full stop. Then the exception list would contain very specific instances of where the guideline applies, but the group agrees that it should not be enforced for some reason.
What I'm trying to prevent is the overuse of exceptions. As an exaggerated example you could instead write this guideline as "all addition shall not be used", and then provide exceptions for "float point arithmetic", "std::opts overloads", "known constant expressions", "adding zero", "operations that be statically determined to not wrap", etc.
``rationale`` ``status`` | ||
------------------------ | ||
|
||
The ``status`` option of a ``rationale`` **MUST** match the ``status`` of its parent ``guideline``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering why the status of the rationale
is separate if it must match the parent? Wouldn't it be better to instead have a parent_id
here? The same comment applies to the following example sections
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a limitation of my knowledge. As far as I could tell in order to have a Need in Sphinx-Needs it's required to at a minimum have:
- an
id
- a
status
If we can figure out another way to do this, I'm fine with not having a status
.
Rationale Content | ||
----------------- | ||
|
||
TODO(pete.levasseur) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's suggestion for kicking off this section.
TODO(pete.levasseur) | |
The rationale **SHOULD** aim to explain to a reader why the guideline is useful in the context of safety critical Rust. The target audience is someone who familiar with Rust, and assumed to have read the contents of the _ The Rust Programming Language_ book, but is otherwise unaware of, or needs to be reminded about specific problematic behaviors that the guideline is attempting to cover. The rationale **MAY** include citations to specific, trusted resources that also explain the guideline, but **MUST** be able to be read coherently without accessing those resources. |
Conveniently, the MISRA C Working Group had a meeting today so we were able to discuss some of this PR and generate some additional comments for the review. First and most importantly, the MISRA C WG doesn't see any IP-related risk with the currently proposed wording. What little has been copied so far clearly either falls under Fair Use* or are simple dictionary terms over which MISRA does not have ownership (e.g. "Amplification" sections - not a common word - which is neat for use as a header - but we certainly didn't invent it). (* or equivalent, since MISRA itself falls under UK copyright) Secondly, the point was made that if the goal of this document is to stay in sync with the same MRAD system and Compliance etc., copying the definitions might create a maintenance burden if we change them in the C document, Obviously the C document is paywalled so it can't be used as a definition reference, but Compliance 2020 does have (slightly differently worded) definitions that can be used, and since it's free, it can be used for definitions easily. (Compliance 2020 does refer to the C document for some terms, which is unfortunate, but this will aim to be fixed in the 2025 update) So the suggestion of the group is to "as defined in Compliance 2020" for the definitions of the terms themselves, and then add longer additional descriptions in the section for guidance for people writing new Rust rules oriented purely towards helping people pick an appropriate category. This separates the concerns a little while having a single normative definition for the actual compliance case. We like that you're doing that for the reference to the description of a deviation process! Thirdly, someone made an interesting point that if a GRP is used to alter the category of any given Guideline, the RFC-2119 words used in the headline and Amplification might not be as-expected. We don't actually think this is a problem, but it might be unexpected at the user end. We don't recommend changing the wording of headlines dynamically (!), but rather having some user-facing note in the intro that RFC-2119 applies "except when a rule has been recategorized, in which case ... etc". We will work to make sure that the next edition of Compliance is accessible and suitable for referencing in this way - it's already supposed to be free and language-agnostic, but we will keep Rust in mind when reviewing any updates, and ensure that minor inconsistencies like Compliance relying on any definitions from MISRA C, are completely removed. If the SCRC has any feedback about Compliance y'all want us to take into account, that would also be very welcome and will be incorporated where possible. |
Thanks for sharing the feedback here! I'll go bit by bit --
Great to hear! Thank you for checking into this.
Got it. Purely my own opinion at the moment follows: I'm a fan of not reinventing the wheel if it can be helped! Plus if we were to show strong alignment with MISRA way of doing things it's my thought this could help with adoption of these coding guidelines. Would the thought be (from your point of view or MISRA members)
? There's also the issue of usage of RFC 2119 that could conflict with this -- but you'd mentioned this is something you'd like to have done given the chance. Wondering how we can square those two things -- using RFC 2119 here while MISRA does not.
Yay!
Sorry about this, but I think the point you're making here went over my head 😅 Let's bring this into the meeting this Wednesday to discuss?
Got it on the stylistic choice. See my question above on how to align these guidelines with the concepts of MISRA while still making use of IETF RFC 2119.
Thanks for the offer! I'll try to read over Compliance and ask others as well to see if there's something we could suggest. |
By the way this build is failing due to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of details that spring to mind.
|
||
A unique identifier for each guideline. Guideline identifiers **MUST** begin with ``gui_``. | ||
|
||
These identifiers are considered **stable** across releases and **MUST NOT** be removed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
across Rust releases? or across Guideline releases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both I'd think.
Retired Guidelines | ||
================== | ||
|
||
.. toctree:: | ||
:maxdepth: 1 | ||
|
||
retired-types-and-traits | ||
retired-patterns | ||
retired-expressions | ||
retired-values | ||
retired-statements | ||
retired-functions | ||
retired-associated-items | ||
retired-implementations | ||
retired-generics | ||
retired-attributes | ||
retired-entities-and-resolution | ||
retired-ownership-and-destruction | ||
retired-exceptions-and-errors | ||
retired-concurrency | ||
retired-program-structure-and-compilation | ||
retired-unsafety | ||
retired-macros | ||
retired-ffi | ||
retired-inline-assembly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it useful to have all of these from the get go? Or should we just add as needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could go either way. I'm thinking to keep them for the structure to be clear.
* use IETF RFC 2119 * add stub for Guideline Lifecycle (status) * add section for deprecated guidelines * add stubbed section for Compliance (deviation process) * add mechanism for which release the guideline is relevant for * add about release to style guideline
…nd status of retired
ca10ef6
to
69b3f7a
Compare
use IETF RFC 2119
add stub for Guideline Lifecycle (status)
add section for deprecated guidelines
add stubbed section for Compliance (deviation process)
add mechanism for which release the guideline is relevant for
add about release to style guideline
closes #25