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

Inherit ScopeFlags::StrictMode from parent scope #3862

Open
overlookmotel opened this issue Jun 23, 2024 · 3 comments
Open

Inherit ScopeFlags::StrictMode from parent scope #3862

overlookmotel opened this issue Jun 23, 2024 · 3 comments
Labels
C-bug Category - Bug

Comments

@overlookmotel
Copy link
Collaborator

overlookmotel commented Jun 23, 2024

There appear to be some discrepancies in when ScopeFlags::StrictMode is applied by semantic.

enter_scope looks like its logic is that StrictMode should be set on any scope which is strict mode (including from a 'use strict' directive somewhere above it). But this doesn't appear to be what it actually does, because it only sets StrictMode flag if the direct parent is a function with a 'use strict' directive, or if the entire program is strict mode:

impl<'a> Visit<'a> for SemanticBuilder<'a> {
fn enter_scope(&mut self, flags: ScopeFlags) {
let parent_scope_id =
if flags.contains(ScopeFlags::Top) { None } else { Some(self.current_scope_id) };
let mut flags = flags;
// Inherit strict mode for functions
// https://tc39.es/ecma262/#sec-strict-mode-code
if let Some(parent_scope_id) = parent_scope_id {
let mut strict_mode = self.scope.root_flags().is_strict_mode();
let parent_scope_flags = self.scope.get_flags(parent_scope_id);
if !strict_mode
&& parent_scope_flags.is_function()
&& parent_scope_flags.is_strict_mode()
{
strict_mode = true;
}
// inherit flags for non-function scopes
if !flags.contains(ScopeFlags::Function) {
flags |= parent_scope_flags & ScopeFlags::Modifiers;
};
if strict_mode {
flags |= ScopeFlags::StrictMode;
}

A couple of examples of weirdness:

// Script - sloppy mode
function foo() {
  'use strict';
  {
    // Scope flags for this block include `StrictMode`
    {
      // Scope flags for this block do NOT include `StrictMode`
    }
  }
}
// Script - sloppy mode
class X {
  foo() {
    // Scope flags for this method do NOT include `StrictMode`
  }
}

When is StrictMode flag meant to be set?

If intended behavior is that StrictMode flag should be set for any scope which runs in strict context, we can simplify the above logic to:

if let Some(parent_scope_id) = parent_scope_id {
    let parent_scope_flags = self.scope.get_flags(parent_scope_id);
    flags |= parent_scope_flags & ScopeFlags::StrictMode;

    // inherit flags for non-function scopes
    if !flags.contains(ScopeFlags::Function) {
        flags |= parent_scope_flags & ScopeFlags::Modifiers;
    }
}
@overlookmotel overlookmotel added the C-bug Category - Bug label Jun 23, 2024
@overlookmotel
Copy link
Collaborator Author

NB: #3908 moves the logic above to oxc_semantic/src/scope.rs

@Boshen
Copy link
Member

Boshen commented Jun 29, 2024

strict mode should be inherited, seems like a bug in places that are not checked.

This can be verified by

// It is a Syntax Error if this phrase is contained in strict mode code and the StringValue of IdentifierName is: "implements", "interface", "let", "package", "private", "protected", "public", "static", or "yield".
if ctx.strict_mode() && STRICT_MODE_NAMES.contains(name) {
ctx.error(reserved_keyword(name, span));
}

where strictness is checked on the scope.

@overlookmotel overlookmotel changed the title When should ScopeFlags::StrictMode be set? Inherit ScopeFlags::StrictMode from parent scope Jul 1, 2024
@overlookmotel
Copy link
Collaborator Author

Thanks for clarifying. I think we can fix with:

  1. the replacement mentioned above.
  2. set StrictMode flag on all classes (NB extends clause runs in strict mode, as well as the class body, but I'm not sure about decorators).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category - Bug
Projects
None yet
Development

No branches or pull requests

2 participants