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(linter): implement isolated_declaration #209

Merged
merged 4 commits into from
Mar 26, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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]
1 │ export function foo(a) { return a; }
· ─
╰────

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

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

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

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

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

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

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

⚠ isolated-declaration: Requires return type annotation of non-private method
╭─[isolated_declaration.tsx:1:1]
1 │ export 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