Skip to content
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

feat: Add deprecated attribute #2041

Merged
merged 6 commits into from Aug 1, 2023
Merged

feat: Add deprecated attribute #2041

merged 6 commits into from Aug 1, 2023

Conversation

ghost
Copy link

@ghost ghost commented Jul 26, 2023

Description

fn main() {
    deprecated();
}

#[deprecated]
fn deprecated() {}
> nargo check
warning: use of deprecated function deprecated
  ┌─ /home/work/subject/oliver/noir/app/src/main.nr:2:5
  │
2 │     deprecated();
  │     ----------

Constraint system successfully built!

Problem

Resolves #879

PR Checklist

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

@ghost ghost changed the title impl deprecated attribute feat: add deprecated attribute Jul 26, 2023
Copy link
Member

@TomAFrench TomAFrench left a comment

Choose a reason for hiding this comment

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

Can you add tests showing that we can display custom deprecation messages which should be defined inside the #[deprecated] tag? We should check that these are usable on foreign/oracle/builtin functions as well.

crates/noirc_frontend/src/hir/type_check/errors.rs Outdated Show resolved Hide resolved
@kevaundray kevaundray changed the title feat: add deprecated attribute feat: Add deprecated attribute Jul 26, 2023
@kevaundray
Copy link
Contributor

Agree that we should add tests to see how this plays with other attributes.

Also agree, it would be good to have the reason for deprecation, seems to be quite easy to add it like we have with Oracle attribute for example and retain that information until we need to emit a warming. It is not hard blocking for this PR though (wasn't in the issue that it resolves)

@ghost
Copy link
Author

ghost commented Jul 26, 2023

As of now, only one attribute per function is currently supported.

// XXX: Currently we only have one attribute defined. If more attributes are needed per function, we can make this a vector and make attribute definition more expressive
pub attribute: Option<Attribute>,

I'm not sure how to implement it correctly, but I'll give it a try.

@kevaundray
Copy link
Contributor

As of now, only one attribute per function is currently supported.

// XXX: Currently we only have one attribute defined. If more attributes are needed per function, we can make this a vector and make attribute definition more expressive
pub attribute: Option<Attribute>,

I'm not sure how to implement it correctly, but I'll give it a try.

We can open up another issue for multiple attributes, as that may introduce scope creap. Can you just look into adding the message?

@ghost ghost requested a review from TomAFrench July 26, 2023 08:41
@TomAFrench
Copy link
Member

Can you add a test which shows that this warning gets created correctly when parsing a program?

crates/noirc_frontend/src/hir/type_check/expr.rs Outdated Show resolved Hide resolved
crates/noirc_frontend/src/hir/type_check/expr.rs Outdated Show resolved Hide resolved
crates/noirc_frontend/src/hir/type_check/errors.rs Outdated Show resolved Hide resolved
jfecher
jfecher previously approved these changes Jul 27, 2023
@jfecher jfecher enabled auto-merge July 27, 2023 15:56
@jfecher
Copy link
Contributor

jfecher commented Jul 27, 2023

@TomAFrench pending re-review

auto-merge was automatically disabled July 28, 2023 14:52

Head branch was pushed to by a user without write access

Copy link
Member

@TomAFrench TomAFrench left a comment

Choose a reason for hiding this comment

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

To match Rust, we should do #[deprecated = "message"] rather than #[deprecated("message")] https://doc.rust-lang.org/reference/attributes/diagnostics.html#the-deprecated-attribute

We'd probably want to adjust this for the other attributes as well though I think so this can be pushed into a diff PR.

@TomAFrench TomAFrench added this pull request to the merge queue Aug 1, 2023
Merged via the queue into noir-lang:master with commit 9e2cf6f Aug 1, 2023
TomAFrench added a commit that referenced this pull request Aug 1, 2023
* master: (53 commits)
  chore: Update `noir-source-resolver` to v1.1.3 (#1912)
  chore: Document `GeneratedAcir::more_than_eq_comparison` (#2085)
  chore: refresh ACIR test artifacts (#2091)
  feat: Add `deprecated` attribute (#2041)
  chore(ssa refactor): Implement `acir_gen` errors (#2071)
  chore: use witnesses from the generated acir in the ABI (#2095)
  fix: Fix methods not mutating fields (#2087)
  chore(nargo): Use Display impl for InputValue (#1990)
  feat: Make arrays and slices polymorphic over each other (#2070)
  feat: Remove an unnecessary witness in `mul_with_witness` (#2078)
  chore: document truncate (#2082)
  fix: avoid potential panic in `two_complement` (#2081)
  chore: Cleanup integration tests (#2074)
  chore: replace `Type::TypeVariable`, `Type::PolymorphicInteger`, and … (#2065)
  chore!: Require package names in `Nargo.toml` files (#2056)
  fix: Avoid non-determinism in defunctionalization (#2069)
  chore: change 'unnecessary pub' error to a warning (#2064)
  feat!: Update to ACVM 0.21.0 (#2051)
  chore: Rename execute tests for an accurate description (#2063)
  chore: Restore lost integration test (#2062)
  ...
TomAFrench added a commit that referenced this pull request Aug 1, 2023
* master: (75 commits)
  fix: Mutating a variable no longer mutates its copy (#2057)
  fix: Implement `.len()` in Acir-Gen (#2077)
  chore: clippy fixes (#2101)
  chore: Update `noir-source-resolver` to v1.1.3 (#1912)
  chore: Document `GeneratedAcir::more_than_eq_comparison` (#2085)
  chore: refresh ACIR test artifacts (#2091)
  feat: Add `deprecated` attribute (#2041)
  chore(ssa refactor): Implement `acir_gen` errors (#2071)
  chore: use witnesses from the generated acir in the ABI (#2095)
  fix: Fix methods not mutating fields (#2087)
  chore(nargo): Use Display impl for InputValue (#1990)
  feat: Make arrays and slices polymorphic over each other (#2070)
  feat: Remove an unnecessary witness in `mul_with_witness` (#2078)
  chore: document truncate (#2082)
  fix: avoid potential panic in `two_complement` (#2081)
  chore: Cleanup integration tests (#2074)
  chore: replace `Type::TypeVariable`, `Type::PolymorphicInteger`, and … (#2065)
  chore!: Require package names in `Nargo.toml` files (#2056)
  fix: Avoid non-determinism in defunctionalization (#2069)
  chore: change 'unnecessary pub' error to a warning (#2064)
  ...
TomAFrench added a commit that referenced this pull request Aug 1, 2023
* master: (75 commits)
  fix: Mutating a variable no longer mutates its copy (#2057)
  fix: Implement `.len()` in Acir-Gen (#2077)
  chore: clippy fixes (#2101)
  chore: Update `noir-source-resolver` to v1.1.3 (#1912)
  chore: Document `GeneratedAcir::more_than_eq_comparison` (#2085)
  chore: refresh ACIR test artifacts (#2091)
  feat: Add `deprecated` attribute (#2041)
  chore(ssa refactor): Implement `acir_gen` errors (#2071)
  chore: use witnesses from the generated acir in the ABI (#2095)
  fix: Fix methods not mutating fields (#2087)
  chore(nargo): Use Display impl for InputValue (#1990)
  feat: Make arrays and slices polymorphic over each other (#2070)
  feat: Remove an unnecessary witness in `mul_with_witness` (#2078)
  chore: document truncate (#2082)
  fix: avoid potential panic in `two_complement` (#2081)
  chore: Cleanup integration tests (#2074)
  chore: replace `Type::TypeVariable`, `Type::PolymorphicInteger`, and … (#2065)
  chore!: Require package names in `Nargo.toml` files (#2056)
  fix: Avoid non-determinism in defunctionalization (#2069)
  chore: change 'unnecessary pub' error to a warning (#2064)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add deprecation attribute to functions
3 participants