feat(linter): add CFGLintContext for rules with #[use_cfg].#3747
Conversation
Your org has enabled the Graphite merge queue for merging into mainAdd the label “merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix. You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link. |
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
CodSpeed Performance ReportMerging #3747 will degrade performances by 3.42%Comparing Summary
Benchmarks breakdown
|
|
|
Boshen
force-pushed
the
06-18-perf_linter_only_generate_cfg_when_we_have_a_rule_which_depends_on_it
branch
from
be0dd5b to
7ac80eb
Compare
Boshen
force-pushed
the
06-19-feat_linter_add_cfglintcontext_for_rules_with_use_cfg_
branch
from
0ddedbf to
c575a19
Compare
| severity: Severity, | ||
| } | ||
|
|
||
| #[derive(Clone)] |
There was a problem hiding this comment.
I think the transmute between &LintContext/&CFGLintContext and &LintContextData is sound as you've made both structs #[repr(transparent)].
But if someone changed the type defs without understanding this need, it'd lead to UB.
To make this unlikely to happen, I think it needs a comment here at the location of the type definitions explaining what is going on here, the motivation for it, and saying at minimum "It is essential that LintContext and LintContextData have identical memory layouts so that From impls below are sound.".
|
Closing the stack as too complicated. |
…g enabled semantic. (#3761) It has the same spirit as #3747 but with a much simpler approach. I've used the fact that @Boshen mentioned about linter always using CFG so now we assert `semantic.cfg().is_some()` in the `LintContext::new` because of this assertion we can have a `LintContext::cfg` that unwraps unchecked. Eliminates unnecessary checks in our hot paths. It has the best of both worlds, No complicated typing yet we still get the CFG as a non-optional value without extra ASM or branching.
…g enabled semantic. (#3761) It has the same spirit as #3747 but with a much simpler approach. I've used the fact that @Boshen mentioned about linter always using CFG so now we assert `semantic.cfg().is_some()` in the `LintContext::new` because of this assertion we can have a `LintContext::cfg` that unwraps unchecked. Eliminates unnecessary checks in our hot paths. It has the best of both worlds, No complicated typing yet we still get the CFG as a non-optional value without extra ASM or branching.
Boshen
deleted the
06-19-feat_linter_add_cfglintcontext_for_rules_with_use_cfg_
branch
3 participants
related to this #3744 (comment)
Now only rules declared with the
#[use_cfg]attribute would get to usectx.cfgmethod otherwise they have to check forcfx.semantic().cfg()to beSome.If this gets merged we should be able to unwrap the control flow unchecked; Since our current linter runner is getting its context from outside we should check for this not being
Nonebefore iterating and panicking early on.