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: let all compiler errors carry a Location instead of a Span #7486

Merged
merged 18 commits into from
Feb 25, 2025

Conversation

asterite
Copy link
Collaborator

@asterite asterite commented Feb 21, 2025

Description

Problem

Resolves #7452
Resolves #7453

Summary

Another huge PR, but this one is simpler than the other one (it's more mechanical).

Now that most of the compiler code works with Location instead of Span, this PR makes all compiler errors carry a Location instead of a Span (when they carried spans).

With this, a lot is simplified:

  1. When pushing errors we no longer need to specify in which file they happen, because each error knows its file
  2. Consequently, the errors we track are just Vec<CompilationError> instead of Vec<(CompilationError, FileId)> because all errors have a location and so have a file.
  3. In fact, the field file from Elaborator is no longer needed, so bugs that would happen because it wasn't updated can't happen anymore
  4. Errors that mentioned multiple spans from potentially different files now mention multiple locations so their files can naturally be different

An example of 4 is this code:

// main.nr
mod other;

use other::Foo;

impl Foo {
    fn foo(self) {}
}

fn main() {}

// other.nr
pub struct Foo {}

impl Foo {
    pub fn foo(self) {}
}

The old error is:

error: duplicate definitions of foo found
  ┌─ src/other.nr:4:12
  │
4 │     pub fn foo(self) {}
  │            ---   --- first definition found here
  │            │
  │            second definition found here

The new error is:

error: duplicate definitions of foo found
  ┌─ src/main.nr:6:8
  │
6 │     fn foo(self) {}
  │        --- first definition found here
  │
  ┌─ src/other.nr:4:12
  │
4 │     pub fn foo(self) {}
  │            --- second definition found here

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.

@asterite asterite changed the title chore: let all compiler errors carry a Location instead of a Span feat: let all compiler errors carry a Location instead of a Span Feb 21, 2025
@asterite asterite requested a review from a team February 21, 2025 18:41
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.

Remaining TODO but otherwise LGTM

Optionally, the two Location's in ExpressionKind::Unsafe could get mixed up so you could:

  • Move one of the Location's to BlockExpression
  • Make ExpressionKind::Unsafe a struct variant

@asterite
Copy link
Collaborator Author

@michaeljklein I created an UnsafeExpression struct. In the end only one of the locations ended up being used, but I think with the struct it's a bit clearer so I left it.

Copy link
Contributor

@jfecher jfecher left a comment

Choose a reason for hiding this comment

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

LGTM - this change has been a long time coming.

Looks like we'll need to bump the maximum memory limit for private-kernel-tail

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

@asterite asterite added this pull request to the merge queue Feb 25, 2025
Merged via the queue into master with commit 4fdc742 Feb 25, 2025
103 checks passed
@asterite asterite deleted the ab/compiler-diagnostic-location branch February 25, 2025 20:04
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Feb 26, 2025
feat: let all compiler errors carry a Location instead of a Span (noir-lang/noir#7486)
chore: Increaes base64's allotted time (noir-lang/noir#7521)
fix: don't crash on broken impl syntax (noir-lang/noir#7512)
feat: optimize out range checks on limiting cases (noir-lang/noir#7510)
chore: clippy fixes (noir-lang/noir#7505)
chore(docs): Supplement docs on `modexp` as a required precompile for Barretenberg's Solidity verifier (noir-lang/noir#7508)
feat(debugger): REPL add breakpoint by sourcecode line (noir-lang/noir#5204)
fix: issue duplicate error on impl function without self (noir-lang/noir#7490)
feat(experimental): Support struct constructors in match patterns (noir-lang/noir#7489)
feat: use resolved type instead of needing Constructor.struct_type (noir-lang/noir#7500)
feat: better error message when keyword is found instead of type in p… (noir-lang/noir#7501)
chore: bump external pinned commits (noir-lang/noir#7497)
feat(experimental): Add invalid pattern syntax error (noir-lang/noir#7487)
fix(performance): Accurately mark safe constant indices for arrays of complex types (noir-lang/noir#7491)
fix(experimental): Allow shadowing in match patterns (noir-lang/noir#7484)
chore: regression test #7195 (noir-lang/noir#7233)
chore(docs): Section on `noir-profiler execution-opcodes` (noir-lang/noir#7480)
chore: improve proptesting of 128bit values in `noirc_abi` (noir-lang/noir#7485)
chore(profiler): Use brillig names for outputted flamegraphs  (noir-lang/noir#7470)
chore(docs): Profiler images reference (noir-lang/noir#7481)
fix: don't use dummy location when inserting debug code (noir-lang/noir#7482)
feat(cli)!: Add `--unstable-features` to gate unstable features (noir-lang/noir#7449)
TomAFrench pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Feb 26, 2025
Automated pull of development from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
chore: bump external pinned commits
(noir-lang/noir#7515)
feat: let all compiler errors carry a Location instead of a Span
(noir-lang/noir#7486)
chore: Increaes base64's allotted time
(noir-lang/noir#7521)
fix: don't crash on broken impl syntax
(noir-lang/noir#7512)
feat: optimize out range checks on limiting cases
(noir-lang/noir#7510)
chore: clippy fixes (noir-lang/noir#7505)
chore(docs): Supplement docs on `modexp` as a required precompile for
Barretenberg's Solidity verifier
(noir-lang/noir#7508)
feat(debugger): REPL add breakpoint by sourcecode line
(noir-lang/noir#5204)
fix: issue duplicate error on impl function without self
(noir-lang/noir#7490)
feat(experimental): Support struct constructors in match patterns
(noir-lang/noir#7489)
feat: use resolved type instead of needing Constructor.struct_type
(noir-lang/noir#7500)
feat: better error message when keyword is found instead of type in p…
(noir-lang/noir#7501)
chore: bump external pinned commits
(noir-lang/noir#7497)
feat(experimental): Add invalid pattern syntax error
(noir-lang/noir#7487)
fix(performance): Accurately mark safe constant indices for arrays of
complex types (noir-lang/noir#7491)
fix(experimental): Allow shadowing in match patterns
(noir-lang/noir#7484)
chore: regression test #7195
(noir-lang/noir#7233)
chore(docs): Section on `noir-profiler execution-opcodes`
(noir-lang/noir#7480)
chore: improve proptesting of 128bit values in `noirc_abi`
(noir-lang/noir#7485)
chore(profiler): Use brillig names for outputted flamegraphs
(noir-lang/noir#7470)
chore(docs): Profiler images reference
(noir-lang/noir#7481)
fix: don't use dummy location when inserting debug code
(noir-lang/noir#7482)
feat(cli)!: Add `--unstable-features` to gate unstable features
(noir-lang/noir#7449)
END_COMMIT_OVERRIDE

---------

Co-authored-by: guipublic <[email protected]>
Co-authored-by: guipublic <[email protected]>
@asterite asterite mentioned this pull request Feb 27, 2025
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants