Skip to content

Commit

Permalink
feat(linter): implement isolated_declaration (#209)
Browse files Browse the repository at this point in the history
* feat(linter): implement isolated_declaration

* Implement check for class elements

* Set up Export SymbolFlag and store declaration AST in Symbol

* Fix tests and comments
  • Loading branch information
YangchenYe323 authored Mar 26, 2023
1 parent ef19895 commit 929b0ef
Show file tree
Hide file tree
Showing 14 changed files with 357 additions and 65 deletions.
16 changes: 16 additions & 0 deletions crates/oxc_linter/src/ast_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -186,3 +186,19 @@ impl<'a, 'b> IsConstant<'a, 'b> for SpreadElement<'a> {
self.argument.is_constant(in_boolean_position, ctx)
}
}

pub trait IsPrivate {
fn is_private(&self) -> bool;
}

impl<'a> IsPrivate for MethodDefinition<'a> {
fn is_private(&self) -> bool {
self.accessibility.map_or(false, |accessibility| accessibility == TSAccessibility::Private)
}
}

impl<'a> IsPrivate for PropertyDefinition<'a> {
fn is_private(&self) -> bool {
self.accessibility.map_or(false, |accessibility| accessibility == TSAccessibility::Private)
}
}
3 changes: 2 additions & 1 deletion crates/oxc_linter/src/rules.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,5 +25,6 @@ oxc_macros::declare_all_lint_rules! {
no_unsafe_negation,
deepscan::uninvoked_array_callback,
use_isnan,
valid_typeof
valid_typeof,
typescript::isolated_declaration
}
155 changes: 155 additions & 0 deletions crates/oxc_linter/src/rules/typescript/isolated_declaration.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,155 @@
use oxc_ast::{
ast::{Class, ClassElement, Function, PropertyDefinition},
AstKind, Span,
};
use oxc_diagnostics::{
miette::{self, Diagnostic},
thiserror::Error,
};
use oxc_macros::declare_oxc_lint;
use oxc_semantic::Symbol;

use crate::{ast_util::IsPrivate, context::LintContext, rule::Rule};

#[derive(Debug, Error, Diagnostic)]
enum IsolatedDeclarationDiagnostic {
#[error("isolated-declaration: Requires type annotation on non-private property definition")]
#[diagnostic(severity(warning))]
Property(#[label] Span),
#[error("isolated-declaration: Requires type annotation on parameters of non-private method")]
#[diagnostic(severity(warning))]
FunctionParam(#[label] Span),
#[error("isolated-declaration: Requires return type annotation of non-private method")]
#[diagnostic(severity(warning))]
FunctionReturnType(#[label] Span),
}

#[derive(Debug, Default, Clone)]
pub struct IsolatedDeclaration;

declare_oxc_lint!(
/// ### What it does
/// This rule enforces a set of restrictions on typescript files to enable "isolated declaration",
/// i.e., .d.ts files can be generated from a single .ts file without resolving its dependencies.
/// The typescript implementation is at `https://github.com/microsoft/TypeScript/pull/53463`
/// The thread on isolated declaration is at `https://github.com/microsoft/TypeScript/issues/47947`
///
/// ### Why is this bad?
/// The restrictions allow .d.ts files to be generated based on one single .ts file, which improves the
/// efficiency and possible parallelism of the declaration emitting process. Furthermore, it prevents syntax
/// errors in one file from propagating to other dependent files.
///
/// ### Example
/// ```typescript
/// export class Test {
/// x // error under isolated declarations
/// private y = 0; // no error, private field types are not serialized in declarations
/// #z = 1;// no error, fields is not present in declarations
/// constructor(x: number) {
/// this.x = 1;
/// }
/// get a() { // error under isolated declarations
/// return 1;
/// }
/// set a(value) { // error under isolated declarations
/// this.x = 1;
/// }
/// }
/// ```
IsolatedDeclaration,
nursery,
);

impl Rule for IsolatedDeclaration {
fn run_on_symbol(&self, symbol: &Symbol, ctx: &LintContext<'_>) {
if symbol.is_export() {
let declaration = ctx.nodes()[symbol.declaration()];
match declaration.kind() {
AstKind::Class(class) => Self::check_class(class, ctx),
AstKind::Function(function) => Self::check_function(function, ctx),
_ => (),
}
}
}
}

impl IsolatedDeclaration {
/// Checks that:
/// 1. all the parameters of the function has type annotation
/// 2. return type of the function has type annotation
pub fn check_function(function: &Function, ctx: &LintContext<'_>) {
let parameters = &function.params.items;
for param in parameters {
if param.pattern.type_annotation.is_none() {
ctx.diagnostic(IsolatedDeclarationDiagnostic::FunctionParam(param.span));
}
}
if function.return_type.is_none() {
ctx.diagnostic(IsolatedDeclarationDiagnostic::FunctionReturnType(function.span));
}
}

/// Checks that the property as a type annotation
pub fn check_property_definition(property: &PropertyDefinition, ctx: &LintContext<'_>) {
if property.type_annotation.is_none() {
ctx.diagnostic(IsolatedDeclarationDiagnostic::Property(property.span));
}
}

/// Checks that:
/// 1. All the non private methods are valid by `Self::check_function`
/// 2. All the non private class fields have a type annotation
/// 3. All the non private variables have a type annotation
pub fn check_class(class: &Class, ctx: &LintContext<'_>) {
for element in &class.body.body {
match element {
ClassElement::MethodDefinition(method) => {
if !method.is_private() {
Self::check_function(&method.value, ctx);
}
}
ClassElement::PropertyDefinition(property) => {
if !property.is_private() && !property.key.is_private_identifier() {
Self::check_property_definition(property, ctx);
}
}
ClassElement::TSAbstractMethodDefinition(method) => {
Self::check_function(&method.method_definition.value, ctx);
}
ClassElement::TSAbstractPropertyDefinition(property) => {
Self::check_property_definition(&property.property_definition, ctx);
}
ClassElement::StaticBlock(_)
| ClassElement::AccessorProperty(_)
| ClassElement::TSIndexSignature(_) => (),
}
}
}
}

#[test]
fn test() {
use crate::tester::Tester;

let pass = vec![
("export function foo(a: number): number { return a; }", None),
("export class A { public a: number; }", None),
("export class A { private a; }", None),
("export class A { #a; }", None),
("export class A { foo(): number { return 0; } }", None),
("export class A { private foo() { return 0; } }", None),
("export class A { public foo(a: number): number { return a; } }", None),
];

let fail = vec![
("export function foo(a) { return a; }", None),
("export class A { public a; }", None),
("export class A { foo() { return 0; } }", None),
("export abstract class A { abstract foo() { return 0; } }", None),
("export abstract class A { abstract a; }", None),
("export class A { get foo() { return 0; } }", None),
("export class A { set foo(val) { } }", None),
];

Tester::new(IsolatedDeclaration::NAME, pass, fail).test_and_snapshot();
}
59 changes: 59 additions & 0 deletions crates/oxc_linter/src/snapshots/isolated_declaration.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
---
source: crates/oxc_linter/src/tester.rs
expression: isolated_declaration
---

isolated-declaration: Requires type annotation on parameters of non-private method
╭─[isolated_declaration.tsx:1:1]
1export function foo(a) { return a; }
· ─
╰────

isolated-declaration: Requires return type annotation of non-private method
╭─[isolated_declaration.tsx:1:1]
1export function foo(a) { return a; }
· ─────────────────────────────
╰────

isolated-declaration: Requires type annotation on non-private property definition
╭─[isolated_declaration.tsx:1:1]
1export class A { public a; }
· ─────────
╰────

isolated-declaration: Requires return type annotation of non-private method
╭─[isolated_declaration.tsx:1:1]
1export class A { foo() { return 0; } }
· ────────────────
╰────

isolated-declaration: Requires return type annotation of non-private method
╭─[isolated_declaration.tsx:1:1]
1export abstract class A { abstract foo() { return 0; } }
· ────────────────
╰────

isolated-declaration: Requires type annotation on non-private property definition
╭─[isolated_declaration.tsx:1:1]
1export abstract class A { abstract a; }
· ───────────
╰────

isolated-declaration: Requires return type annotation of non-private method
╭─[isolated_declaration.tsx:1:1]
1export class A { get foo() { return 0; } }
· ────────────────
╰────

isolated-declaration: Requires type annotation on parameters of non-private method
╭─[isolated_declaration.tsx:1:1]
1export class A { set foo(val) { } }
· ───
╰────

isolated-declaration: Requires return type annotation of non-private method
╭─[isolated_declaration.tsx:1:1]
1export class A { set foo(val) { } }
· ─────────
╰────

8 changes: 6 additions & 2 deletions crates/oxc_parser/src/ts/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -395,8 +395,12 @@ impl<'a> Parser<'a> {
self.parse_ts_declare_function(start_span, modifiers)
.map(Declaration::FunctionDeclaration)
} else if self.ts_enabled() {
self.parse_ts_function_impl(start_span, FunctionKind::TSDeclaration, modifiers)
.map(Declaration::FunctionDeclaration)
self.parse_ts_function_impl(
start_span,
FunctionKind::Declaration { single_statement: true },
modifiers,
)
.map(Declaration::FunctionDeclaration)
} else {
self.parse_function_impl(FunctionKind::Declaration { single_statement: true })
.map(Declaration::FunctionDeclaration)
Expand Down
4 changes: 1 addition & 3 deletions crates/oxc_semantic/src/binder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -155,9 +155,7 @@ impl<'a> Binder for CatchClause<'a> {
// unless CatchParameter is CatchParameter : BindingIdentifier
if let BindingPatternKind::BindingIdentifier(ident) = &param.kind {
let includes = SymbolFlags::FunctionScopedVariable | SymbolFlags::CatchVariable;
// Overshadows declarations so redeclarator error is not reported here
let symbol_id = builder.symbols.create(ident.name.clone(), ident.span, includes);
builder.scope.current_scope_mut().variables.insert(ident.name.clone(), symbol_id);
builder.declare_shadow_symbol(&ident.name, ident.span, current_scope_id, includes);
} else {
for ident in param.bound_names() {
builder.declare_symbol(
Expand Down
31 changes: 30 additions & 1 deletion crates/oxc_semantic/src/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ pub struct SemanticBuilder<'a> {
// states
pub current_node_id: AstNodeId,
pub current_node_flags: NodeFlags,
pub current_symbol_flags: SymbolFlags,

// builders
pub nodes: AstNodes<'a>,
Expand Down Expand Up @@ -58,6 +59,7 @@ impl<'a> SemanticBuilder<'a> {
errors: vec![],
current_node_id,
current_node_flags: NodeFlags::empty(),
current_symbol_flags: SymbolFlags::empty(),
nodes,
scope,
symbols: SymbolTable::default(),
Expand Down Expand Up @@ -147,7 +149,22 @@ impl<'a> SemanticBuilder<'a> {
if let Some(symbol_id) = self.check_redeclaration(scope_id, name, span, excludes) {
return symbol_id;
}
let symbol_id = self.symbols.create(name.clone(), span, includes);
let includes = includes | self.current_symbol_flags;
let symbol_id = self.symbols.create(self.current_node_id, name.clone(), span, includes);
self.scope.scopes[scope_id].variables.insert(name.clone(), symbol_id);
symbol_id
}

/// Declares a `Symbol` for the node, shadowing previous declarations in the same scope.
pub fn declare_shadow_symbol(
&mut self,
name: &Atom,
span: Span,
scope_id: ScopeId,
includes: SymbolFlags,
) -> SymbolId {
let includes = includes | self.current_symbol_flags;
let symbol_id = self.symbols.create(self.current_node_id, name.clone(), span, includes);
self.scope.scopes[scope_id].variables.insert(name.clone(), symbol_id);
symbol_id
}
Expand Down Expand Up @@ -193,6 +210,7 @@ impl<'a> SemanticBuilder<'a> {
fn enter_kind(&mut self, kind: AstKind<'a>) {
match kind {
AstKind::ModuleDeclaration(decl) => {
self.current_symbol_flags |= Self::symbol_flag_from_module_declaration(decl);
decl.bind(self);
}
AstKind::VariableDeclarator(decl) => {
Expand Down Expand Up @@ -233,6 +251,9 @@ impl<'a> SemanticBuilder<'a> {
AstKind::Class(_) => {
self.current_node_flags -= NodeFlags::Class;
}
AstKind::ModuleDeclaration(decl) => {
self.current_symbol_flags -= Self::symbol_flag_from_module_declaration(decl);
}
_ => {}
}
}
Expand Down Expand Up @@ -267,4 +288,12 @@ impl<'a> SemanticBuilder<'a> {
}
}
}

fn symbol_flag_from_module_declaration(module: &ModuleDeclaration) -> SymbolFlags {
if matches!(&module.kind, ModuleDeclarationKind::ImportDeclaration(_)) {
SymbolFlags::Import
} else {
SymbolFlags::Export
}
}
}
Loading

0 comments on commit 929b0ef

Please sign in to comment.