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(semantic): check for illegal symbol modifiers #3838

Merged
merged 6 commits into from
Jun 23, 2024
Merged
Show file tree
Hide file tree
Changes from 5 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
11 changes: 10 additions & 1 deletion crates/oxc_ast/src/ast/ts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1339,7 +1339,16 @@ impl<'a> Modifiers<'a> {
.map_or(false, |modifiers| modifiers.iter().any(|modifier| modifier.kind == target))
}

pub fn find<F>(&self, f: F) -> Option<&Modifier>
pub fn iter(&self) -> impl Iterator<Item = &Modifier> + '_ {
self.0.as_ref().into_iter().flat_map(|modifiers| modifiers.iter())
}

/// Find a modifier by kind
pub fn find(&self, kind: ModifierKind) -> Option<&Modifier> {
self.find_where(|modifier| modifier.kind == kind)
}

pub fn find_where<F>(&self, f: F) -> Option<&Modifier>
where
F: Fn(&Modifier) -> bool,
{
Expand Down
5 changes: 3 additions & 2 deletions crates/oxc_linter/src/rules/oxc/no_const_enum.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,9 @@ declare_oxc_lint!(
impl Rule for NoConstEnum {
fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) {
if let AstKind::TSEnumDeclaration(enum_decl) = node.kind() {
let Some(const_enum) =
enum_decl.modifiers.find(|modifier| matches!(modifier.kind, ModifierKind::Const))
let Some(const_enum) = enum_decl
.modifiers
.find_where(|modifier| matches!(modifier.kind, ModifierKind::Const))
else {
return;
};
Expand Down
18 changes: 14 additions & 4 deletions crates/oxc_semantic/src/checker/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ pub fn check<'a>(node: &AstNode<'a>, ctx: &SemanticBuilder<'a>) {
AstKind::RegExpLiteral(lit) => js::check_regexp_literal(lit, ctx),

AstKind::Directive(dir) => js::check_directive(dir, ctx),
AstKind::Function(func) => ts::check_function(func, node, ctx),
AstKind::ModuleDeclaration(decl) => {
js::check_module_declaration(decl, node, ctx);
}
Expand Down Expand Up @@ -68,8 +69,10 @@ pub fn check<'a>(node: &AstNode<'a>, ctx: &SemanticBuilder<'a>) {
js::check_function_declaration(alternate, true, ctx);
}
}

AstKind::Class(class) => js::check_class(class, node, ctx),
AstKind::Class(class) => {
js::check_class(class, node, ctx);
ts::check_class(class, node, ctx);
}
AstKind::MethodDefinition(method) => js::check_method_definition(method, ctx),
AstKind::ObjectProperty(prop) => js::check_object_property(prop, ctx),
AstKind::Super(sup) => js::check_super(sup, node, ctx),
Expand All @@ -91,13 +94,20 @@ pub fn check<'a>(node: &AstNode<'a>, ctx: &SemanticBuilder<'a>) {
AstKind::ObjectExpression(expr) => js::check_object_expression(expr, ctx),
AstKind::UnaryExpression(expr) => js::check_unary_expression(expr, node, ctx),
AstKind::YieldExpression(expr) => js::check_yield_expression(expr, node, ctx),
AstKind::VariableDeclaration(decl) => ts::check_variable_declaration(decl, node, ctx),
AstKind::VariableDeclarator(decl) => ts::check_variable_declarator(decl, ctx),
AstKind::SimpleAssignmentTarget(target) => ts::check_simple_assignment_target(target, ctx),
AstKind::TSTypeParameterDeclaration(declaration) => {
ts::check_ts_type_parameter_declaration(declaration, ctx);
}
AstKind::TSModuleDeclaration(decl) => ts::check_ts_module_declaration(decl, ctx),
AstKind::TSEnumDeclaration(decl) => ts::check_ts_enum_declaration(decl, ctx),
AstKind::TSModuleDeclaration(decl) => ts::check_ts_module_declaration(decl, node, ctx),
AstKind::TSEnumDeclaration(decl) => ts::check_ts_enum_declaration(decl, node, ctx),
AstKind::TSTypeAliasDeclaration(decl) => {
ts::check_ts_type_alias_declaration(decl, node, ctx);
}
AstKind::TSInterfaceDeclaration(decl) => {
ts::check_ts_interface_declaration(decl, node, ctx);
}
_ => {}
}
}
106 changes: 102 additions & 4 deletions crates/oxc_semantic/src/checker/typescript.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,10 @@ use oxc_ast::syntax_directed_operations::BoundNames;
use oxc_ast::{ast::*, AstKind};
use oxc_diagnostics::OxcDiagnostic;
use oxc_span::{Atom, GetSpan, Span};
use oxc_syntax::scope::ScopeFlags;
use rustc_hash::FxHashMap;

use crate::{builder::SemanticBuilder, diagnostics::redeclaration};
use crate::{builder::SemanticBuilder, diagnostics::redeclaration, AstNode};

fn empty_type_parameter_list(span0: Span) -> OxcDiagnostic {
OxcDiagnostic::error("Type parameter list cannot be empty.").with_labels([span0.into()])
Expand All @@ -23,7 +24,6 @@ pub fn check_ts_type_parameter_declaration(
fn unexpected_optional(span0: Span) -> OxcDiagnostic {
OxcDiagnostic::error("Unexpected `?` operator").with_labels([span0.into()])
}

#[allow(clippy::cast_possible_truncation)]
pub fn check_variable_declarator(decl: &VariableDeclarator, ctx: &SemanticBuilder<'_>) {
if decl.id.optional {
Expand Down Expand Up @@ -114,14 +114,107 @@ pub fn check_array_pattern<'a>(pattern: &ArrayPattern<'a>, ctx: &SemanticBuilder
}
}

/// ts(1184)
fn modifiers_cannot_be_used_here(span: Span) -> OxcDiagnostic {
OxcDiagnostic::error("Modifiers cannot be used here.").with_labels([span.into()])
}

/// ts(1024)
fn readonly_only_on_index_signature_or_property_decl(span: Span) -> OxcDiagnostic {
OxcDiagnostic::error(
"'readonly' modifier can only appear on a property declaration or index signature.",
)
.with_labels([span.into()])
}
/// ts(1042)
fn async_cannot_be_used_here(span: Span) -> OxcDiagnostic {
OxcDiagnostic::error("'async' modifier cannot be used here.").with_labels([span.into()])
}

/// Returns `true` if the scope described by `scope_flags` supports export
/// declarations or specifiers.
pub const fn scope_can_export(scope_flags: ScopeFlags) -> bool {
const CAN_EXPORT: ScopeFlags = ScopeFlags::Top.union(ScopeFlags::TsModuleBlock);
scope_flags.contains(CAN_EXPORT)
}

fn check_declaration_modifiers<'a>(
modifiers: &Modifiers<'a>,
decl_node: &AstNode<'a>,
ctx: &SemanticBuilder<'a>,
) {
let scope_flags = ctx.scope.get_flags(decl_node.scope_id());
let kind = decl_node.kind();
let is_class = matches!(kind, AstKind::Class(_));
let is_function = matches!(kind, AstKind::Function(_));
for modifier in modifiers.iter() {
match modifier.kind {
ModifierKind::Public
| ModifierKind::Private
| ModifierKind::Protected
| ModifierKind::Static
| ModifierKind::Override => {
ctx.error(modifiers_cannot_be_used_here(modifier.span));
}
ModifierKind::Abstract if !is_class => {
ctx.error(modifiers_cannot_be_used_here(modifier.span));
}
ModifierKind::Async if !is_function => {
ctx.error(async_cannot_be_used_here(modifier.span));
}
ModifierKind::Readonly => {
ctx.error(readonly_only_on_index_signature_or_property_decl(modifier.span));
}
ModifierKind::Export if !scope_can_export(scope_flags) => {
ctx.error(modifiers_cannot_be_used_here(modifier.span));
}
_ => {}
}
}
}
pub fn check_variable_declaration<'a>(
decl: &VariableDeclaration<'a>,
node: &AstNode<'a>,
ctx: &SemanticBuilder<'a>,
) {
check_declaration_modifiers(&decl.modifiers, node, ctx);
}

pub fn check_function<'a>(function: &Function<'a>, node: &AstNode<'a>, ctx: &SemanticBuilder<'a>) {
check_declaration_modifiers(&function.modifiers, node, ctx);
}
pub fn check_class<'a>(class: &Class<'a>, node: &AstNode<'a>, ctx: &SemanticBuilder<'a>) {
check_declaration_modifiers(&class.modifiers, node, ctx);
}

pub fn check_ts_type_alias_declaration<'a>(
decl: &TSTypeAliasDeclaration<'a>,
node: &AstNode<'a>,
ctx: &SemanticBuilder<'a>,
) {
check_declaration_modifiers(&decl.modifiers, node, ctx);
}
pub fn check_ts_interface_declaration<'a>(
decl: &TSInterfaceDeclaration<'a>,
node: &AstNode<'a>,
ctx: &SemanticBuilder<'a>,
) {
check_declaration_modifiers(&decl.modifiers, node, ctx);
}

fn not_allowed_namespace_declaration(span0: Span) -> OxcDiagnostic {
OxcDiagnostic::error(
"A namespace declaration is only allowed at the top level of a namespace or module.",
)
.with_labels([span0.into()])
}

pub fn check_ts_module_declaration<'a>(decl: &TSModuleDeclaration<'a>, ctx: &SemanticBuilder<'a>) {
pub fn check_ts_module_declaration<'a>(
decl: &TSModuleDeclaration<'a>,
node: &AstNode<'a>,
ctx: &SemanticBuilder<'a>,
) {
check_declaration_modifiers(&decl.modifiers, node, ctx);
// skip current node
for node in ctx.nodes.iter_parents(ctx.current_node_id).skip(1) {
match node.kind() {
Expand All @@ -144,8 +237,13 @@ fn enum_member_must_have_initializer(span0: Span) -> OxcDiagnostic {
OxcDiagnostic::error("Enum member must have initializer.").with_labels([span0.into()])
}

pub fn check_ts_enum_declaration(decl: &TSEnumDeclaration<'_>, ctx: &SemanticBuilder<'_>) {
pub fn check_ts_enum_declaration<'a>(
decl: &TSEnumDeclaration<'a>,
node: &AstNode<'a>,
ctx: &SemanticBuilder<'a>,
) {
let mut need_initializer = false;
check_declaration_modifiers(&decl.modifiers, node, ctx);

decl.members.iter().for_each(|member| {
#[allow(clippy::unnested_or_patterns)]
Expand Down
55 changes: 54 additions & 1 deletion crates/oxc_semantic/tests/integration/modules.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use oxc_semantic::SymbolFlags;
use oxc_semantic::{SemanticBuilderReturn, SymbolFlags};

use crate::util::SemanticTester;

Expand Down Expand Up @@ -46,6 +46,23 @@ fn test_exported_named_function() {
.has_some_symbol("T")
.is_not_exported()
.test();

SemanticTester::tsx(
"
import React from 'react';
export const Counter: React.FC<{ count: number }> = ({ count }) => (
<div>{count}</div>
)
",
)
.has_some_symbol("Counter")
.is_exported()
.contains_flags(
SymbolFlags::ConstVariable
.union(SymbolFlags::BlockScopedVariable)
.union(SymbolFlags::Export),
)
.test();
}

#[test]
Expand Down Expand Up @@ -149,3 +166,39 @@ fn test_exported_interface() {
test.has_some_symbol("a").is_not_exported().test();
test.has_some_symbol("T").is_not_exported().test();
}

#[test]
fn test_exports_in_namespace() {
let test = SemanticTester::ts(
"
export const x = 1;
namespace N {
function foo() {
return 1
}
export function bar() {
return foo();
}
export const x = 2
}
",
);
test.has_some_symbol("bar").is_exported().test();
let semantic = test.build();
assert!(!semantic.module_record().exported_bindings.contains_key("bar"));
}

#[test]
fn test_export_in_invalid_scope() {
let test = SemanticTester::js(
"
function foo() {
export const x = 1;
}",
)
.expect_errors(true);
test.has_some_symbol("x").contains_flags(SymbolFlags::Export).test();
let SemanticBuilderReturn { semantic, errors } = test.build_with_errors();
assert!(!errors.is_empty(), "expected an export within a function to produce a check error, but no errors were produced");
assert!(semantic.module_record().exported_bindings.is_empty());
}
17 changes: 17 additions & 0 deletions crates/oxc_semantic/tests/integration/symbols.rs
Original file line number Diff line number Diff line change
Expand Up @@ -114,3 +114,20 @@ fn test_export_flag() {
tester.has_root_symbol("b").contains_flags(SymbolFlags::Export).test();
tester.has_root_symbol("c").contains_flags(SymbolFlags::Export).test();
}

#[test]
fn test_invalid_modifiers() {
const PARAM_PROPERTY: &str =
"A parameter property is only allowed in a constructor implementation.";
const ILLEGAL_MODIFIER: &str = "Modifiers cannot be used here.";
const READONLY: &str =
"'readonly' modifier can only appear on a property declaration or index signature.";

SemanticTester::ts("function foo(public x: number) { }").has_error(PARAM_PROPERTY);
// SemanticTester::ts("function foo() { export const x = 1; }").has_error(illegal_modifier);
SemanticTester::ts("function foo() { public const x = 1; }").has_error(ILLEGAL_MODIFIER);
SemanticTester::ts("function foo() { private const x = 1; }").has_error(ILLEGAL_MODIFIER);
SemanticTester::ts("function foo() { protected const x = 1; }").has_error(ILLEGAL_MODIFIER);
SemanticTester::ts("function foo() { abstract const x = 1; }").has_error(ILLEGAL_MODIFIER);
SemanticTester::ts("function foo() { readonly const x = 1; }").has_error(READONLY);
}
Loading