Skip to content

Commit

Permalink
fix!: Change tag attributes to require a ' prefix (#6235)
Browse files Browse the repository at this point in the history
# Description

## Problem\*

## Summary\*

Changes tag attributes (previously Custom attributes) to require a `'`
prefix. This distinguishes them from the more common Meta attributes
which are expected to call a metaprogramming function.

The program:

```rs
#[not_in_scope]
fn main() {}
```
Now errors with:
```
error: Attribute function `not_in_scope` is not in scope
  ┌─ src/main.nr:1:3
  │
1 │ #[not_in_scope]
  │   ------------
  │
```

## Additional Context

- It turns out the `#[deprecated = ...]` attribute was not being handled
previously. I'm not sure if this was expected but it's now easier to see
now that we get an error if it does not get a `'` prefix.
- This also pointed out that `derive` was not working within the stdlib
in the `meta/ops.nr` file. `derive` was never imported there. Moreover,
after being used we get errors since derive tries to insert
`std::cmp::Ordering` paths which are invalid within the stdlib where
they should be `crate::cmp::Ordering` instead. So I replaced these calls
with explicit impls.

## Documentation\*

Check one:
- [x] No documentation needed.
- We may possibly need to document Tag attributes, but I'm unsure who
the user would be.
- [ ] Documentation included in this PR.
- [ ] **[For Experimental Features]** Documentation to be submitted in a
separate PR.

# PR Checklist\*

- [x] I have tested the changes locally.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
  • Loading branch information
jfecher authored Oct 8, 2024
1 parent 9a12f31 commit b43dcb2
Show file tree
Hide file tree
Showing 22 changed files with 164 additions and 62 deletions.
2 changes: 1 addition & 1 deletion compiler/noirc_driver/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -445,7 +445,7 @@ fn compile_contract_inner(
.secondary
.iter()
.filter_map(|attr| {
if let SecondaryAttribute::Custom(attribute) = attr {
if let SecondaryAttribute::Tag(attribute) = attr {
Some(&attribute.contents)
} else {
None
Expand Down
2 changes: 1 addition & 1 deletion compiler/noirc_frontend/src/ast/visitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1391,7 +1391,7 @@ impl SecondaryAttribute {
}

pub fn accept_children(&self, target: AttributeTarget, visitor: &mut impl Visitor) {
if let SecondaryAttribute::Custom(custom) = self {
if let SecondaryAttribute::Meta(custom) = self {
custom.accept(target, visitor);
}
}
Expand Down
36 changes: 28 additions & 8 deletions compiler/noirc_frontend/src/elaborator/comptime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ impl<'context> Elaborator<'context> {
attribute_context: AttributeContext,
generated_items: &mut CollectedItems,
) {
if let SecondaryAttribute::Custom(attribute) = attribute {
if let SecondaryAttribute::Meta(attribute) = attribute {
self.elaborate_in_comptime_context(|this| {
if let Err(error) = this.run_comptime_attribute_name_on_item(
&attribute.contents,
Expand Down Expand Up @@ -188,25 +188,45 @@ impl<'context> Elaborator<'context> {

let location = Location::new(attribute_span, self.file);
let Some((function, arguments)) = Self::parse_attribute(attribute, location)? else {
// Do not issue an error if the attribute is unknown
return Ok(());
return Err((
ResolverError::UnableToParseAttribute {
attribute: attribute.to_string(),
span: attribute_span,
}
.into(),
self.file,
));
};

// Elaborate the function, rolling back any errors generated in case it is unknown
let error_count = self.errors.len();
let function_string = function.to_string();
let function = self.elaborate_expression(function).0;
self.errors.truncate(error_count);

let definition_id = match self.interner.expression(&function) {
HirExpression::Ident(ident, _) => ident.id,
_ => return Ok(()),
_ => {
return Err((
ResolverError::AttributeFunctionIsNotAPath {
function: function_string,
span: attribute_span,
}
.into(),
self.file,
))
}
};

let Some(definition) = self.interner.try_definition(definition_id) else {
// If there's no such function, don't return an error.
// This preserves backwards compatibility in allowing custom attributes that
// do not refer to comptime functions.
return Ok(());
return Err((
ResolverError::AttributeFunctionNotInScope {
name: function_string,
span: attribute_span,
}
.into(),
self.file,
));
};

let DefinitionKind::Function(function) = definition.kind else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2242,7 +2242,7 @@ fn function_def_add_attribute(
}
}

if let Attribute::Secondary(SecondaryAttribute::Custom(attribute)) = attribute {
if let Attribute::Secondary(SecondaryAttribute::Tag(attribute)) = attribute {
let func_meta = interpreter.elaborator.interner.function_meta_mut(&func_id);
func_meta.custom_attributes.push(attribute);
}
Expand Down
27 changes: 27 additions & 0 deletions compiler/noirc_frontend/src/hir/resolution/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,12 @@ pub enum ResolverError {
UnsupportedNumericGenericType(#[from] UnsupportedNumericGenericType),
#[error("Type `{typ}` is more private than item `{item}`")]
TypeIsMorePrivateThenItem { typ: String, item: String, span: Span },
#[error("Unable to parse attribute `{attribute}`")]
UnableToParseAttribute { attribute: String, span: Span },
#[error("Attribute function `{function}` is not a path")]
AttributeFunctionIsNotAPath { function: String, span: Span },
#[error("Attribute function `{name}` is not in scope")]
AttributeFunctionNotInScope { name: String, span: Span },
}

impl ResolverError {
Expand Down Expand Up @@ -546,6 +552,27 @@ impl<'a> From<&'a ResolverError> for Diagnostic {
*span,
)
},
ResolverError::UnableToParseAttribute { attribute, span } => {
Diagnostic::simple_error(
format!("Unable to parse attribute `{attribute}`"),
"Attribute should be a function or function call".into(),
*span,
)
},
ResolverError::AttributeFunctionIsNotAPath { function, span } => {
Diagnostic::simple_error(
format!("Attribute function `{function}` is not a path"),
"An attribute's function should be a single identifier or a path".into(),
*span,
)
},
ResolverError::AttributeFunctionNotInScope { name, span } => {
Diagnostic::simple_error(
format!("Attribute function `{name}` is not in scope"),
String::new(),
*span,
)
},
}
}
}
19 changes: 12 additions & 7 deletions compiler/noirc_frontend/src/lexer/lexer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -301,6 +301,11 @@ impl<'a> Lexer<'a> {
}
self.next_char();

let is_tag = self.peek_char_is('\'');
if is_tag {
self.next_char();
}

let contents_start = self.position + 1;

let word = self.eat_while(None, |ch| ch != ']');
Expand All @@ -321,7 +326,7 @@ impl<'a> Lexer<'a> {
let span = Span::inclusive(start, end);
let contents_span = Span::inclusive(contents_start, contents_end);

let attribute = Attribute::lookup_attribute(&word, span, contents_span)?;
let attribute = Attribute::lookup_attribute(&word, span, contents_span, is_tag)?;
if is_inner {
match attribute {
Attribute::Function(attribute) => Err(LexerErrorKind::InvalidInnerAttribute {
Expand Down Expand Up @@ -838,17 +843,17 @@ mod tests {
}

#[test]
fn custom_attribute() {
let input = r#"#[custom(hello)]"#;
fn tag_attribute() {
let input = r#"#['custom(hello)]"#;
let mut lexer = Lexer::new(input);

let token = lexer.next_token().unwrap();
assert_eq!(
token.token(),
&Token::Attribute(Attribute::Secondary(SecondaryAttribute::Custom(CustomAttribute {
&Token::Attribute(Attribute::Secondary(SecondaryAttribute::Tag(CustomAttribute {
contents: "custom(hello)".to_string(),
span: Span::from(0..16),
contents_span: Span::from(2..15)
span: Span::from(0..17),
contents_span: Span::from(3..16)
})))
);
}
Expand Down Expand Up @@ -943,7 +948,7 @@ mod tests {
let token = lexer.next_token().unwrap();
assert_eq!(
token.token(),
&Token::InnerAttribute(SecondaryAttribute::Custom(CustomAttribute {
&Token::InnerAttribute(SecondaryAttribute::Meta(CustomAttribute {
contents: "something".to_string(),
span: Span::from(0..13),
contents_span: Span::from(3..12),
Expand Down
30 changes: 24 additions & 6 deletions compiler/noirc_frontend/src/lexer/token.rs
Original file line number Diff line number Diff line change
Expand Up @@ -738,6 +738,7 @@ impl Attribute {
word: &str,
span: Span,
contents_span: Span,
is_tag: bool,
) -> Result<Attribute, LexerErrorKind> {
let word_segments: Vec<&str> = word
.split(|c| c == '(' || c == ')')
Expand All @@ -758,6 +759,14 @@ impl Attribute {
is_valid.ok_or(LexerErrorKind::MalformedFuncAttribute { span, found: word.to_owned() })
};

if is_tag {
return Ok(Attribute::Secondary(SecondaryAttribute::Tag(CustomAttribute {
contents: word.to_owned(),
span,
contents_span,
})));
}

let attribute = match &word_segments[..] {
// Primary Attributes
["foreign", name] => {
Expand Down Expand Up @@ -813,7 +822,7 @@ impl Attribute {
["allow", tag] => Attribute::Secondary(SecondaryAttribute::Allow(tag.to_string())),
tokens => {
tokens.iter().try_for_each(|token| validate(token))?;
Attribute::Secondary(SecondaryAttribute::Custom(CustomAttribute {
Attribute::Secondary(SecondaryAttribute::Meta(CustomAttribute {
contents: word.to_owned(),
span,
contents_span,
Expand Down Expand Up @@ -922,7 +931,13 @@ pub enum SecondaryAttribute {
ContractLibraryMethod,
Export,
Field(String),
Custom(CustomAttribute),

/// A custom tag attribute: #['foo]
Tag(CustomAttribute),

/// An attribute expected to run a comptime function of the same name: #[foo]
Meta(CustomAttribute),

Abi(String),

/// A variable-argument comptime function.
Expand All @@ -939,7 +954,7 @@ pub enum SecondaryAttribute {

impl SecondaryAttribute {
pub(crate) fn as_custom(&self) -> Option<&CustomAttribute> {
if let Self::Custom(attribute) = self {
if let Self::Tag(attribute) = self {
Some(attribute)
} else {
None
Expand All @@ -954,7 +969,8 @@ impl SecondaryAttribute {
}
SecondaryAttribute::Export => Some("export".to_string()),
SecondaryAttribute::Field(_) => Some("field".to_string()),
SecondaryAttribute::Custom(custom) => custom.name(),
SecondaryAttribute::Tag(custom) => custom.name(),
SecondaryAttribute::Meta(custom) => custom.name(),
SecondaryAttribute::Abi(_) => Some("abi".to_string()),
SecondaryAttribute::Varargs => Some("varargs".to_string()),
SecondaryAttribute::UseCallersScope => Some("use_callers_scope".to_string()),
Expand All @@ -977,7 +993,8 @@ impl fmt::Display for SecondaryAttribute {
SecondaryAttribute::Deprecated(Some(ref note)) => {
write!(f, r#"#[deprecated("{note}")]"#)
}
SecondaryAttribute::Custom(ref attribute) => write!(f, "#[{}]", attribute.contents),
SecondaryAttribute::Tag(ref attribute) => write!(f, "#['{}]", attribute.contents),
SecondaryAttribute::Meta(ref attribute) => write!(f, "#[{}]", attribute.contents),
SecondaryAttribute::ContractLibraryMethod => write!(f, "#[contract_library_method]"),
SecondaryAttribute::Export => write!(f, "#[export]"),
SecondaryAttribute::Field(ref k) => write!(f, "#[field({k})]"),
Expand Down Expand Up @@ -1029,7 +1046,8 @@ impl AsRef<str> for SecondaryAttribute {
match self {
SecondaryAttribute::Deprecated(Some(string)) => string,
SecondaryAttribute::Deprecated(None) => "",
SecondaryAttribute::Custom(attribute) => &attribute.contents,
SecondaryAttribute::Tag(attribute) => &attribute.contents,
SecondaryAttribute::Meta(attribute) => &attribute.contents,
SecondaryAttribute::Field(string)
| SecondaryAttribute::Abi(string)
| SecondaryAttribute::Allow(string) => string,
Expand Down
6 changes: 3 additions & 3 deletions compiler/noirc_frontend/src/parser/parser/attributes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,10 +55,10 @@ mod tests {

#[test]
fn parses_inner_attribute() {
let src = "#![hello]";
let src = "#!['hello]";
let mut parser = Parser::for_str(src);
let Some(SecondaryAttribute::Custom(custom)) = parser.parse_inner_attribute() else {
panic!("Expected inner custom attribute");
let Some(SecondaryAttribute::Tag(custom)) = parser.parse_inner_attribute() else {
panic!("Expected inner tag attribute");
};
expect_no_errors(&parser.errors);
assert_eq!(custom.contents, "hello");
Expand Down
16 changes: 16 additions & 0 deletions compiler/noirc_frontend/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3203,3 +3203,19 @@ fn trait_unconstrained_methods_typechecked_correctly() {
println!("{errors:?}");
assert_eq!(errors.len(), 0);
}

#[test]
fn error_if_attribute_not_in_scope() {
let src = r#"
#[not_in_scope]
fn main() {}
"#;

let errors = get_program_errors(src);
assert_eq!(errors.len(), 1);

assert!(matches!(
errors[0].0,
CompilationError::ResolverError(ResolverError::AttributeFunctionNotInScope { .. })
));
}
3 changes: 2 additions & 1 deletion noir_stdlib/src/ec/consts/te.nr
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ pub struct BabyJubjub {
}

#[field(bn254)]
#[deprecated = "It's recommmended to use the external noir-edwards library (https://github.com/noir-lang/noir-edwards)"]
// Uncommenting this results in deprecated warnings in the stdlib
// #[deprecated]
pub fn baby_jubjub() -> BabyJubjub {
BabyJubjub {
// Baby Jubjub (ERC-2494) parameters in affine representation
Expand Down
6 changes: 4 additions & 2 deletions noir_stdlib/src/hash/mimc.nr
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@ use crate::default::Default;
// You must use constants generated for the native field
// Rounds number should be ~ log(p)/log(exp)
// For 254 bit primes, exponent 7 and 91 rounds seems to be recommended
#[deprecated = "It's recommmended to use the external MiMC library (https://github.com/noir-lang/mimc)"]
//
// Uncommenting this results in deprecated warnings in the stdlib
// #[deprecated]
fn mimc<let N: u32>(x: Field, k: Field, constants: [Field; N], exp: Field) -> Field {
//round 0
let mut t = x + k;
Expand Down Expand Up @@ -117,7 +119,7 @@ global MIMC_BN254_CONSTANTS: [Field; MIMC_BN254_ROUNDS] = [

//mimc implementation with hardcoded parameters for BN254 curve.
#[field(bn254)]
#[deprecated = "It's recommmended to use the external MiMC library (https://github.com/noir-lang/mimc)"]
#[deprecated]
pub fn mimc_bn254<let N: u32>(array: [Field; N]) -> Field {
let exponent = 7;
let mut r = 0;
Expand Down
2 changes: 2 additions & 0 deletions noir_stdlib/src/meta/mod.nr
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,8 @@ pub comptime fn make_trait_impl<Env1, Env2>(
}

mod tests {
use crate::meta::derive_via;

// docs:start:quote-example
comptime fn quote_one() -> Quoted {
quote { 1 }
Expand Down
29 changes: 27 additions & 2 deletions noir_stdlib/src/meta/op.nr
Original file line number Diff line number Diff line change
@@ -1,8 +1,22 @@
#[derive(Eq, Hash)]
pub struct UnaryOp {
op: Field
}

// Cannot derive Eq or Hash since they internally use paths
// starting with std:: which is invalid within the same crate.
// We'd need to use crate:: instead.
impl crate::cmp::Eq for UnaryOp {
fn eq(self, other: Self) -> bool {
self.op == other.op
}
}

impl crate::hash::Hash for UnaryOp {
fn hash<H>(self, h: &mut H) where H: crate::hash::Hasher {
self.op.hash(h);
}
}

impl UnaryOp {
// docs:start:is_minus
pub fn is_minus(self) -> bool {
Expand Down Expand Up @@ -46,11 +60,22 @@ impl UnaryOp {
}
}

#[derive(Eq, Hash)]
pub struct BinaryOp {
op: Field
}

impl crate::cmp::Eq for BinaryOp {
fn eq(self, other: Self) -> bool {
self.op == other.op
}
}

impl crate::hash::Hash for BinaryOp {
fn hash<H>(self, h: &mut H) where H: crate::hash::Hasher {
self.op.hash(h);
}
}

impl BinaryOp {
// docs:start:is_add
pub fn is_add(self) -> bool {
Expand Down
Loading

0 comments on commit b43dcb2

Please sign in to comment.