Skip to content

feat: require safety comments instead of safety doc comments#7295

Merged
TomAFrench merged 15 commits intomasterfrom
ab/safety-comment
Feb 14, 2025
Merged

feat: require safety comments instead of safety doc comments#7295
TomAFrench merged 15 commits intomasterfrom
ab/safety-comment

Conversation

@asterite
Copy link
Collaborator

@asterite asterite commented Feb 5, 2025 •
edited
Loading

Description

Problem

Resolves #7273

Summary

Instead of requiring /// Safety: on top of unsafe, with this PR we require // Safety:. This matches how Rust works (for that lint).

In order to avoid introducing a big breaking change, this PR:

  • still allows doc comments on top of unsafe and statements, but warns when that happens
  • in general all misplaced doc comments will now produce a warning instead of an error. This is just temporary so that the formatter can work for this upgrade. We can turn it back to how it was after a week or two.
  • the formatter will turn safety doc comments into comments, so all you have to do to fix the warnings because of this change is just run the formatter

Additional Context

Documentation

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@github-actions github-actions bot added the documentation Improvements or additions to documentation label Feb 5, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Feb 5, 2025 •
edited
Loading

🚀 Deployed on https://67af8e08b7f3d34e861cfd6a--noir-docs.netlify.app

@asterite asterite requested a review from a team February 5, 2025 15:48
michaeljklein
michaeljklein approved these changes Feb 5, 2025
View reviewed changes
Copy link
Contributor

@michaeljklein michaeljklein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with small nit r.e. sAfEtY

@asterite asterite added this pull request to the merge queue Feb 5, 2025
@asterite asterite removed this pull request from the merge queue due to a manual request Feb 5, 2025
@asterite asterite requested a review from TomAFrench February 14, 2025 14:23
@TomAFrench
Copy link
Member

TomAFrench commented Feb 14, 2025

I'm fine with changing this but we should stick to this format going forwads.

@TomAFrench TomAFrench added this pull request to the merge queue Feb 14, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 14, 2025
@asterite asterite added this pull request to the merge queue Feb 14, 2025
@asterite asterite mentioned this pull request Feb 14, 2025
Closed
@asterite
Copy link
Collaborator Author

asterite commented Feb 14, 2025

Captured #7394

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 14, 2025
@TomAFrench TomAFrench enabled auto-merge February 14, 2025 18:08
@TomAFrench
Copy link
Member

TomAFrench commented Feb 14, 2025

I've fixed some formatting issues with new tests in master. I've also expanded the formatter check to run on more of the test programs.

@TomAFrench TomAFrench mentioned this pull request Feb 14, 2025
Closed
@github-actions
Copy link
Contributor

github-actions bot commented Feb 14, 2025

FYI @noir-lang/developerrelations on Noir doc changes.

@TomAFrench TomAFrench added this pull request to the merge queue Feb 14, 2025
Merged via the queue into master with commit e895feb Feb 14, 2025
103 checks passed
@TomAFrench TomAFrench deleted the ab/safety-comment branch February 14, 2025 18:55
@noirwhal noirwhal mentioned this pull request Feb 14, 2025
Merged
TomAFrench added a commit that referenced this pull request Feb 14, 2025
@TomAFrench
Merge branch 'master' into mv/duplicate-brillig-entry-points-called-f…
13e1dc3 
…rom-brillig

* master:
  chore: allow opting in to displaying benchmark comments (#7399)
  chore: box `ExprValue` in `Value` enum (#7388)
  chore: pull out refactored methods from u128 branch (#7385)
  feat: require safety comments instead of safety doc comments (#7295)
  fix(ssa): Do not deduplicate division by a zero constant (#7393)
  chore: document traits required to be in scope (#7387)
  fix: field zero division in brillig (#7386)
  chore: box `ParserError`s in `InterpreterError` (#7373)
  chore: remove unnecessary dereferencing within brillig vm (#7375)
  fix: give "correct" error when trying to use AsTraitPath (#7360)
  chore: avoid u128s in brillig memory (#7363)
  chore: update docs about integer overflows (#7370)
  fix!: Only decrement the counter of an array if its address has not changed (#7297)
  fix: let LSP read `noirfmt.toml` for formatting files (#7355)
  chore: deprecate keccak256 (#7361)
  feat: `FunctionDefinition::as_typed_expr` (#7358)
  feat(performance): Check sub operations against induction variables (#7356)
  chore: avoid doing all brillig integer arithmetic on u128s (#7357)
TomAFrench added a commit that referenced this pull request Feb 14, 2025
@TomAFrench
Merge branch 'master' into tf/u128-hacky
0486ab9 
* master:
  chore: allow opting in to displaying benchmark comments (#7399)
  chore: box `ExprValue` in `Value` enum (#7388)
  chore: pull out refactored methods from u128 branch (#7385)
  feat: require safety comments instead of safety doc comments (#7295)
  fix(ssa): Do not deduplicate division by a zero constant (#7393)
  chore: document traits required to be in scope (#7387)
  fix: field zero division in brillig (#7386)
  chore: box `ParserError`s in `InterpreterError` (#7373)
  chore: remove unnecessary dereferencing within brillig vm (#7375)
AztecBot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Feb 15, 2025
[1 changes] fix: format global attributes (noir-lang/noir#7401)
ede23df 
chore: box `Closure` in `comptime::Value` enum (noir-lang/noir#7400)
chore: allow opting in to displaying benchmark comments (noir-lang/noir#7399)
chore: box `ExprValue` in `Value` enum (noir-lang/noir#7388)
chore: pull out refactored methods from u128 branch (noir-lang/noir#7385)
feat: require safety comments instead of safety doc comments (noir-lang/noir#7295)
@AztecBot AztecBot mentioned this pull request Feb 15, 2025
Merged
AztecBot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Feb 16, 2025
[1 changes] fix: format global attributes (noir-lang/noir#7401)
10a2f77 
chore: box `Closure` in `comptime::Value` enum (noir-lang/noir#7400)
chore: allow opting in to displaying benchmark comments (noir-lang/noir#7399)
chore: box `ExprValue` in `Value` enum (noir-lang/noir#7388)
chore: pull out refactored methods from u128 branch (noir-lang/noir#7385)
feat: require safety comments instead of safety doc comments (noir-lang/noir#7295)
AztecBot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Feb 16, 2025
fix: format global attributes (noir-lang/noir#7401)
2faacd5 
chore: box `Closure` in `comptime::Value` enum (noir-lang/noir#7400)
chore: allow opting in to displaying benchmark comments (noir-lang/noir#7399)
chore: box `ExprValue` in `Value` enum (noir-lang/noir#7388)
chore: pull out refactored methods from u128 branch (noir-lang/noir#7385)
feat: require safety comments instead of safety doc comments (noir-lang/noir#7295)
AztecBot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Feb 17, 2025
[1 changes] fix: format global attributes (noir-lang/noir#7401)
2ecd2a8 
chore: box `Closure` in `comptime::Value` enum (noir-lang/noir#7400)
chore: allow opting in to displaying benchmark comments (noir-lang/noir#7399)
chore: box `ExprValue` in `Value` enum (noir-lang/noir#7388)
chore: pull out refactored methods from u128 branch (noir-lang/noir#7385)
feat: require safety comments instead of safety doc comments (noir-lang/noir#7295)
AztecBot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Feb 17, 2025
fix: format global attributes (noir-lang/noir#7401)
1bebc9f 
chore: box `Closure` in `comptime::Value` enum (noir-lang/noir#7400)
chore: allow opting in to displaying benchmark comments (noir-lang/noir#7399)
chore: box `ExprValue` in `Value` enum (noir-lang/noir#7388)
chore: pull out refactored methods from u128 branch (noir-lang/noir#7385)
feat: require safety comments instead of safety doc comments (noir-lang/noir#7295)
TomAFrench added a commit to AztecProtocol/aztec-packages that referenced this pull request Feb 17, 2025
@AztecBot @TomAFrench
feat: Sync from noir (#12022)
ef462bb 
Automated pull of development from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
fix: format global attributes
(noir-lang/noir#7401)
chore: box `Closure` in `comptime::Value` enum
(noir-lang/noir#7400)
chore: allow opting in to displaying benchmark comments
(noir-lang/noir#7399)
chore: box `ExprValue` in `Value` enum
(noir-lang/noir#7388)
chore: pull out refactored methods from u128 branch
(noir-lang/noir#7385)
feat: require safety comments instead of safety doc comments
(noir-lang/noir#7295)
END_COMMIT_OVERRIDE

---------

Co-authored-by: Tom French <tom@tomfren.ch>
AztecBot added a commit to AztecProtocol/aztec-nr that referenced this pull request Feb 18, 2025
@AztecBot @TomAFrench
feat: Sync from noir (#12022)
c361ddd 
Automated pull of development from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
fix: format global attributes
(noir-lang/noir#7401)
chore: box `Closure` in `comptime::Value` enum
(noir-lang/noir#7400)
chore: allow opting in to displaying benchmark comments
(noir-lang/noir#7399)
chore: box `ExprValue` in `Value` enum
(noir-lang/noir#7388)
chore: pull out refactored methods from u128 branch
(noir-lang/noir#7385)
feat: require safety comments instead of safety doc comments
(noir-lang/noir#7295)
END_COMMIT_OVERRIDE

---------

Co-authored-by: Tom French <tom@tomfren.ch>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TomAFrench TomAFrench Awaiting requested review from TomAFrench

1 more reviewer

@michaeljklein michaeljklein michaeljklein approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

documentation Improvements or additions to documentation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Suggest using a doc comment for the "Safety: " notice when a regular comment is used

3 participants

@asterite @TomAFrench @michaeljklein