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: don't report warnings for dependencies #6926

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

Conversation

asterite
Copy link
Collaborator

@asterite asterite commented Jan 2, 2025

Description

Problem

Resolves #6909

Summary

The way this is done here is:

  1. When collecting all files to parse, we also collect which files belong to root packages
  2. When reporting errors and warnings we drop warnings from files that are not in root packages

Additional Context

The name of the branch has "2" in it because at first I tried a different approach: avoid collecting warnings as soon as we encounter them in dependencies. However, this is much more complex because we can't immediately know if an error is a warning or not (we have to turn it into a Diagnostic first, or introduce a large refactor to track this separately) and also in LSP it makes sense to track errors from dependencies if they are tracked by the editor, so it doesn't make sense to exclude them right away.

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.

Copy link
Contributor

github-actions bot commented Jan 2, 2025

Compilation Report

Program Compilation Time %
sha256_regression 1.368s -1%
regression_4709 0.768s -1%
ram_blowup_regression 15.180s 1%
rollup-root 4.580s 8%
rollup-block-merge 3.660s 1%
rollup-base-public 39.600s 0%
rollup-base-private 20.500s 0%
private-kernel-tail 1.080s 7%
private-kernel-reset 6.982s 3%
private-kernel-inner 2.106s 1%
noir-contracts 85.400s -7%

Copy link
Contributor

github-actions bot commented Jan 2, 2025

Execution Report

Program Execution Time %
sha256_regression 0.100s 0%
regression_4709 0.001s 0%
ram_blowup_regression 0.576s -1%
rollup-root 0.131s 2%
rollup-block-merge 0.129s 4%
rollup-base-public 2.490s 1%
rollup-base-private 1.700s 0%
private-kernel-tail 0.023s 0%
private-kernel-reset 0.388s 0%
private-kernel-inner 0.117s 0%

Copy link
Contributor

github-actions bot commented Jan 2, 2025

Peak Memory Sample

Program Peak Memory
keccak256 78.48M
workspace 123.79M
regression_4709 422.91M
ram_blowup_regression 1.58G
rollup-base-public 10.47G
rollup-base-private 6.57G
private-kernel-tail 201.81M
private-kernel-reset 717.07M
private-kernel-inner 291.88M
parity-root 172.14M

@asterite asterite requested a review from a team January 2, 2025 19:23
@TomAFrench
Copy link
Member

Hmm, forcing the user to pass around another piece of compiler state is a little sad. I'd like to avoid this if possible but would need to think more on this.

@asterite
Copy link
Collaborator Author

asterite commented Jan 2, 2025

The set of root files could be tracked by FileManager so we'd need less things to pass around... though at that point maybe FileManager would be doing more just than a single thing.

@TomAFrench
Copy link
Member

I'm thinking that there's enough information for what we want inside of the Context struct, following this down we can get the ModuleData for a particular crate.

pub def_maps: BTreeMap<CrateId, CrateDefMap>,

pub(crate) modules: Arena<ModuleData>,

pub children: HashMap<Ident, LocalModuleId>,

By reading the ModuleData for a particular crate we can get a tree of all the modules within a particular crate. Working down from the root we can then use logic similar to find_module in order to get all of the FileIds connected to this crate.

fn find_module(
file_manager: &FileManager,
anchor: FileId,
mod_name: &Ident,
) -> Result<FileId, DefCollectorErrorKind> {

We should then be able to generate this list after the fact rather than needing to pass it around externally to the compiler.

Note that this method will mean that we ignore any warnings outside of the current crate as opposed to the current workspace. This seems to be closer to what we want imo as it will avoid a workspace library from having its warnings emitted many times (this also matches up with cargo behaviour).

@TomAFrench
Copy link
Member

TomAFrench commented Jan 3, 2025

We could also track this a little more explicitly inside of the compiler state but the above is a "zero footprint" solution.

Edit: agreed that it would be best to keep FileManager "pure" and avoid it knowing about this.

@asterite
Copy link
Collaborator Author

asterite commented Jan 3, 2025

Sounds good, I'll do what you say in the first comment.

I was thinking that eventually it would be nice for the compiler to be able to compile crates modularly. It's a bit strange that we read all files of all packages and all dependencies in one go. Ideally we'd process a package by reading its files, compiling it, then moving to dependent packages, even being able to parallelize things... though I know that would be a huge refactor, but in the way we'd get "for free" errors and warnings grouped by package.

@asterite
Copy link
Collaborator Author

asterite commented Jan 3, 2025

Almost there! In compile_cmd and debug_cmd there's no Context so I don't know where to get the files from... Those files end up calling compile_program that returns CompilationResult<CompiledProgram>, maybe that type (CompiledProgram) could hold that, or maybe a tuple could be returned... but I think it would make the code messier again.

@TomAFrench
Copy link
Member

We can filter the returned warnings inside of the compiler before we pass it back up to nargo_cli though, no? Is there a reason why we need to do this externally?

@asterite
Copy link
Collaborator Author

asterite commented Jan 3, 2025

Is there a reason why we need to do this externally?

The main reason is that for LSP we'd still want to show warnings in crates that are open in the UI but are not the main crate. Though I guess not doing would be fine too...

@asterite
Copy link
Collaborator Author

asterite commented Jan 3, 2025

It worked! And for some reason it works well in LSP too, maybe the code there is slightly different, or maybe all crates are checked, not sure.

Copy link
Contributor

github-actions bot commented Jan 3, 2025

Execution Memory Report

Program Peak Memory
keccak256 74.61M
workspace 123.78M
regression_4709 315.93M
ram_blowup_regression 512.47M
rollup-base-public 1.18G
rollup-base-private 831.52M
private-kernel-tail 181.99M
private-kernel-reset 255.57M
private-kernel-inner 215.17M

Copy link
Contributor

github-actions bot commented Jan 3, 2025

Compilation Memory Report

Program Peak Memory
keccak256 78.50M
workspace 123.71M
regression_4709 422.91M
ram_blowup_regression 1.58G
rollup-base-public 2.80G
rollup-base-private 1.86G
private-kernel-tail 201.95M
private-kernel-reset 716.58M
private-kernel-inner 292.22M

@TomAFrench
Copy link
Member

I'm getting zero warnings being thrown in aztec-packages when using this PR despite there definitely being warnings that should be thrown.

@asterite
Copy link
Collaborator Author

asterite commented Jan 6, 2025

Good catch, I might have changed something in the end that broke it. Now it's fixed.

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.

Automatically filter out any warnings which are thrown in dependencies
2 participants