[move-compiler] New DiagnosticReporter for collecting diags #20123
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
tnowacki
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
| #[derive(PartialEq, Eq, Clone, Debug)] | ||
| enum WarningFiltersScope_ { | ||
| /// Unsafe and should be used only for internal purposes, such as ide annotations | ||
| Empty, | ||
| Static(&'static WarningFilters), | ||
| Node(Arc<WarningFiltersScopeNode>), | ||
| } |
There was a problem hiding this comment.
Switched FiltersScope to a linked list, mostly here to better demonstrate that the initial creation of a DiagnosticReproter has to copy/borrow--no clones.
| } | ||
|
|
||
| impl DefnContext<'_, '_> { | ||
| pub(super) fn add_diag(&self, diag: Diagnostic) { | ||
| self.env.add_diag(&self.warning_filters_scope, diag); | ||
| self.reporter.add_diag(diag); |
There was a problem hiding this comment.
I kept all of the various delegates I added in #20065, since it will make future refactors maybe a bit easier?
I can remove them if there are strong opinions
| flags: &'env Flags, | ||
| known_filter_names: &'env BTreeMap<DiagnosticsID, (FilterPrefix, FilterName)>, | ||
| diags: &'env RwLock<Diagnostics>, | ||
| ide_information: &'env RwLock<IDEInfo>, | ||
| warning_filters_scope: WarningFiltersScope, |
There was a problem hiding this comment.
Apologies for the re-ordering here messing with the diff.
There was a problem hiding this comment.
I grabbed individual fields from CompilationEnv instead of just having a &'env CompilationEnv because I hope it will help ensure a better separation of abstractions for two reasons:
- It should hopefully prevent feature creep that might occur if there was a direct reference to the env.
- I hope this forces us to correctly construct a
DiagnosticReporter. Ifnewjust took&'env CompilationEnv, it would very easy to create them incorrectly.
There was a problem hiding this comment.
That makes sense to me, and it also frees us from needing the whole environment just for reporting. Maybe we can even get some reuse this way.
| @@ -1,2916 +0,0 @@ | |||
| // Copyright (c) The Move Contributors | |||
There was a problem hiding this comment.
Whoops, embarrassing that this got left in
There was a problem hiding this comment.
It happens :) I only found it because of regex
## Description - Followup to #20065 - Better encapsulates warning filter scopes with diagnostic reporting ## Test plan - ran tests --- ## Release notes Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required. For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates. - [ ] Protocol: - [ ] Nodes (Validators and Full nodes): - [ ] Indexer: - [ ] JSON-RPC: - [ ] GraphQL: - [ ] CLI: - [ ] Rust SDK: - [ ] REST API:
Description
Test plan
Release notes
Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.
For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.