Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

Commit

Permalink
fix(rome_js_analyzer): correctly delegate unresolved references to it…
Browse files Browse the repository at this point in the history
…s parent scope (#4004)

* correctly delegate unresolved references to its parent scope
  • Loading branch information
xunilrj authored Dec 7, 2022
1 parent df86e5e commit 388df4e
Show file tree
Hide file tree
Showing 6 changed files with 160 additions and 19 deletions.
4 changes: 4 additions & 0 deletions crates/rome_js_analyze/tests/specs/style/noArguments.cjs
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
function f() {
console.log(arguments);

for(let i = 0;i < arguments.length; ++i) {
console.log(arguments[i]);
}
}

function f() {
Expand Down
41 changes: 39 additions & 2 deletions crates/rome_js_analyze/tests/specs/style/noArguments.cjs.snap
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@ expression: noArguments.cjs
```js
function f() {
console.log(arguments);

for(let i = 0;i < arguments.length; ++i) {
console.log(arguments[i]);
}
}

function f() {
Expand All @@ -23,8 +27,41 @@ noArguments.cjs:2:17 lint/style/noArguments ━━━━━━━━━━━━
1 │ function f() {
> 2 │ console.log(arguments);
│ ^^^^^^^^^
3 │ }
4 │
3 │
4 │ for(let i = 0;i < arguments.length; ++i) {
i arguments does not have Array.prototype methods and can be inconvenient to use.
```

```
noArguments.cjs:4:23 lint/style/noArguments ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
! Use the rest parameters instead of arguments.
2 │ console.log(arguments);
3 │
> 4 │ for(let i = 0;i < arguments.length; ++i) {
│ ^^^^^^^^^
5 │ console.log(arguments[i]);
6 │ }
i arguments does not have Array.prototype methods and can be inconvenient to use.
```

```
noArguments.cjs:5:21 lint/style/noArguments ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
! Use the rest parameters instead of arguments.
4 │ for(let i = 0;i < arguments.length; ++i) {
> 5 │ console.log(arguments[i]);
│ ^^^^^^^^^
6 │ }
7 │ }
i arguments does not have Array.prototype methods and can be inconvenient to use.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,27 @@ export type Invalid<S extends number> = `
```
# Diagnostics
```
noUndeclaredVariables.ts:1:50 lint/correctness/noUndeclaredVariables FIXABLE ━━━━━━━━━━━━━━━━━━━━━
! The T variable is undeclared
> 1 │ export type Invalid<S extends number> = `Hello ${T}`
│ ^
2 │
3 │ export type Invalid<S extends number> = `
i Safe fix: Suppress rule lint/correctness/noUndeclaredVariables
1 │ - export·type·Invalid<S·extends·number>·=·`Hello·${T}`
1 │ + export·type·Invalid<S·extends·number>·=·`Hello·${//·rome-ignore·lint/correctness/noUndeclaredVariables:·<explanation>
2+ T}`
2 3 │
3 4 │ export type Invalid<S extends number> = `
```
```
noUndeclaredVariables.ts:5:7 lint/correctness/noUndeclaredVariables FIXABLE ━━━━━━━━━━━━━━━━━━━━━━
Expand Down
5 changes: 3 additions & 2 deletions crates/rome_js_semantic/src/events.rs
Original file line number Diff line number Diff line change
Expand Up @@ -606,7 +606,7 @@ impl SemanticEventExtractor {

if let Some(scope) = self.scopes.pop() {
// Match references and declarations
for (name, references) in scope.references {
for (name, mut references) in scope.references {
// If we know the declaration of these reference push the correct events...
if let Some(declaration_at) = self.bindings.get(&name) {
for reference in references {
Expand Down Expand Up @@ -647,7 +647,8 @@ impl SemanticEventExtractor {
}
} else if let Some(parent) = self.scopes.last_mut() {
// ... if not, promote these references to the parent scope ...
parent.references.insert(name, references);
let parent_references = parent.references.entry(name).or_default();
parent_references.append(&mut references);
} else {
// ... or raise UnresolvedReference if this is the global scope.
for reference in references {
Expand Down
96 changes: 83 additions & 13 deletions crates/rome_js_semantic/src/tests/assertions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ struct UniqueAssertion {
}

#[derive(Clone, Debug)]
struct UnmatchedAssertion {
struct UnresolvedReferenceAssertion {
range: TextRange,
}

Expand All @@ -252,7 +252,7 @@ enum SemanticAssertion {
AtScope(AtScopeAssertion),
NoEvent(NoEventAssertion),
Unique(UniqueAssertion),
Unmatched(UnmatchedAssertion),
UnresolvedReference(UnresolvedReferenceAssertion),
}

impl SemanticAssertion {
Expand Down Expand Up @@ -335,9 +335,11 @@ impl SemanticAssertion {
range: token.parent().unwrap().text_range(),
}))
} else if assertion_text.contains("/*?") {
Some(SemanticAssertion::Unmatched(UnmatchedAssertion {
range: token.parent().unwrap().text_range(),
}))
Some(SemanticAssertion::UnresolvedReference(
UnresolvedReferenceAssertion {
range: token.parent().unwrap().text_range(),
},
))
} else {
None
}
Expand All @@ -354,7 +356,7 @@ struct SemanticAssertions {
scope_end_assertions: Vec<ScopeEndAssertion>,
no_events: Vec<NoEventAssertion>,
uniques: Vec<UniqueAssertion>,
unmatched: Vec<UnmatchedAssertion>,
unresolved_references: Vec<UnresolvedReferenceAssertion>,
}

impl SemanticAssertions {
Expand All @@ -367,7 +369,7 @@ impl SemanticAssertions {
let mut scope_end_assertions = vec![];
let mut no_events = vec![];
let mut uniques = vec![];
let mut unmatched = vec![];
let mut unresolved_references = vec![];

for node in root
.syntax()
Expand Down Expand Up @@ -419,8 +421,8 @@ impl SemanticAssertions {
Some(SemanticAssertion::Unique(assertion)) => {
uniques.push(assertion);
}
Some(SemanticAssertion::Unmatched(assertion)) => {
unmatched.push(assertion);
Some(SemanticAssertion::UnresolvedReference(assertion)) => {
unresolved_references.push(assertion);
}
None => {}
};
Expand All @@ -437,7 +439,7 @@ impl SemanticAssertions {
scope_end_assertions,
no_events,
uniques,
unmatched,
unresolved_references,
}
}

Expand Down Expand Up @@ -718,26 +720,94 @@ impl SemanticAssertions {
}
}

// Check every unmatched assertion
// Check every unresolved_reference assertion
let is_unresolved_reference =
|e: &SemanticEvent| matches!(e, SemanticEvent::UnresolvedReference { .. });

for unmatched in self.unmatched.iter() {
match events_by_pos.get(&unmatched.range.start()) {
for unresolved_reference in self.unresolved_references.iter() {
match events_by_pos.get(&unresolved_reference.range.start()) {
Some(v) => {
let ok = v
.iter()
.any(|e| matches!(e, SemanticEvent::UnresolvedReference { .. }));
if !ok {
show_all_events(test_name, code, events_by_pos, is_unresolved_reference);
show_unmatched_assertion(
test_name,
code,
unresolved_reference,
unresolved_reference.range,
);
panic!("No UnresolvedReference event found");
}
}
None => {
show_all_events(test_name, code, events_by_pos, is_unresolved_reference);
show_unmatched_assertion(
test_name,
code,
unresolved_reference,
unresolved_reference.range,
);
panic!("No UnresolvedReference event found");
}
}
}
}
}

fn show_unmatched_assertion(
test_name: &str,
code: &str,
assertion: &impl std::fmt::Debug,
assertion_range: TextRange,
) {
let diagnostic = TestSemanticDiagnostic::new(
format!("This assertion was not matched: {assertion:?}"),
assertion_range,
);
let error = diagnostic
.with_file_path((test_name.to_string(), FileId::zero()))
.with_file_source_code(code);

let mut console = EnvConsole::default();
console.log(markup! {
{PrintDiagnostic::verbose(&error)}
});
}

fn show_all_events<F>(
test_name: &str,
code: &str,
events_by_pos: HashMap<TextSize, Vec<SemanticEvent>>,
f: F,
) where
F: Fn(&SemanticEvent) -> bool,
{
let mut console = EnvConsole::default();
let mut all_events = vec![];
for (_, events) in events_by_pos {
for e in events {
if f(&e) {
all_events.push(e);
}
}
}

all_events.sort_by_key(|l| l.range().start());

for e in all_events {
let diagnostic = TestSemanticDiagnostic::new(format!("{e:?}"), e.range());
let error = diagnostic
.with_file_path((test_name.to_string(), FileId::zero()))
.with_file_source_code(code);

console.log(markup! {
{PrintDiagnostic::verbose(&error)}
});
}
}

fn error_assertion_not_attached_to_a_declaration(
code: &str,
assertion_range: TextRange,
Expand Down
12 changes: 10 additions & 2 deletions crates/rome_js_semantic/src/tests/references.rs
Original file line number Diff line number Diff line change
Expand Up @@ -194,8 +194,16 @@ assert_semantics! {
}

assert_semantics! {
ok_unmatched_reference, r#"a/*?*/"#,
ok_function_expression_read,"let f/*#F*/ = function g/*#G*/(){}; g/*?*/();",
ok_unresolved_reference, r#"a/*?*/"#,
ok_unresolved_function_expression_read,"let f/*#F*/ = function g/*#G*/(){}; g/*?*/();",
ok_unresolved_reference_arguments,
r#"function f() {
console.log(arguments/*?*/);
for(let i = 0;i < arguments/*?*/.length; ++i) {
console.log(arguments/*?*/[i]);
}
}"#,
}

// Exports
Expand Down

0 comments on commit 388df4e

Please sign in to comment.