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

Defining, parsing, and checking #[attribute]s #6986

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

ironcev
Copy link
Member

@ironcev ironcev commented Mar 4, 2025

Description

This PR reimplements how #[attribute]s are parsed, defined, and checked. It fixes the following issues we had with attributes:

  • attributes were in general not checked for their expected and allowed usage.
  • when checked, checks were fixing particular reported issues and were scattered through various compilation phases: lexing, parsing, and tree conversion.
  • when checked, reported errors in some cases had bugs and were overalapping with other errors and warnings.
  • attribute handling was not centralized, but rahter scattered throught the whole code base, and mostly done at use sites.

For concrete examples of these issues, see the list of closed issues below.

The PR:

  • encapsulates the attributes handling within a single abstraction: Attributes.
  • assigns the following properties to every attribute:
    • target: what kind of elements an attribute can annotate.
    • multiplicity: if more then one attribute of the same kind can be applied to an element.
    • arguments multiplicity: a range defining how many arguments an attribute can or must have.
    • arguments value expectation: if attribute arguments are expected to have values.
  • validates those properties in a single place, during the convert_parse_tree. Note that this means that modules with attribute issues will now consistently not reach the type checking phase. This was previously the case for some of the issues with attributes where custom checks where done during the type checking phase. Move attribute checks to type checking phase #6987 proposes to move those checks after the tree conversion phase, allowing the continuation of compilation.
  • adds the AttributeKind::Unknow to preserve unknown attributes in the typed tree.
  • removes redundant code related to attributes: specific warnings and errors, specialized parsing, repetitive and error-prone at use site checks, etc.
  • removes the unused Doc attribute.

The PR also introduces a new pattern in emitting errors. The attr_decls_to_attributes function will always return Attributes even if they have errors, because:

  • we expect the compilation to continue even in the case of errors.
  • we expect those errors to be ignored if a valid #[cfg] attribute among those evaluates to false.
  • we expect them to be emitted with eventual other errors, or alone, at the end of the tree conversion of the annotated element.

For more details, see the comment on attr_decls_to_attributes and cfg_eval.

Closes #6880, #6913, #6914, #6915, #6916, #6917, #6918, #6931, #6981, #6983, #6984, #6985.

Breaking changes

Strictly seen, this PR can cause breaking changes, but only in the case of invalid existing attribute usage. We treat those breaking changes as bug fixes in the compiler and, thus, do not have them behind a feature flag.

E.g., this kind of code was possible before, but will now emit and error:

#[storage(read, write, read, write)]
struct Struct {}

Checklist

  • I have linked to any relevant issues.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation where relevant (API docs, the reference, and the Sway book).
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added (or requested a maintainer to add) the necessary Breaking* or New Feature labels where relevant.
  • I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
  • I have requested a review from the relevant team or maintainers.

@ironcev ironcev temporarily deployed to fuel-sway-bot March 4, 2025 15:59 — with GitHub Actions Inactive
Copy link

codspeed-hq bot commented Mar 4, 2025

CodSpeed Performance Report

Merging #6986 will not alter performance

Comparing ironcev/declarative-and-checked-annotations (40bab02) with master (13621ae)

Summary

✅ 22 untouched benchmarks

@ironcev ironcev temporarily deployed to fuel-sway-bot March 4, 2025 16:31 — with GitHub Actions Inactive
@ironcev ironcev temporarily deployed to fuel-sway-bot March 4, 2025 16:41 — with GitHub Actions Inactive
@ironcev ironcev self-assigned this Mar 4, 2025
@ironcev ironcev added compiler General compiler. Should eventually become more specific as the issue is triaged compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen compiler: parser Everything to do with the parser labels Mar 4, 2025
@ironcev ironcev added the breaking May cause existing user code to break. Requires a minor or major release. label Mar 4, 2025
@ironcev ironcev marked this pull request as ready for review March 4, 2025 17:18
@ironcev ironcev requested review from a team as code owners March 4, 2025 17:18
@ironcev ironcev temporarily deployed to fuel-sway-bot March 5, 2025 08:22 — with GitHub Actions Inactive
@ironcev ironcev temporarily deployed to fuel-sway-bot March 6, 2025 08:12 — with GitHub Actions Inactive
@ironcev ironcev temporarily deployed to fuel-sway-bot March 6, 2025 11:30 — with GitHub Actions Inactive
@ironcev ironcev temporarily deployed to fuel-sway-bot March 6, 2025 12:28 — with GitHub Actions Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking May cause existing user code to break. Requires a minor or major release. compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen compiler: parser Everything to do with the parser compiler General compiler. Should eventually become more specific as the issue is triaged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Forbid using attributes where there are not supposed to be used
1 participant