Skip to content

Conversation

@rogeralsing
Copy link
Contributor

@rogeralsing rogeralsing commented Jan 6, 2026

Reverts #425

Summary by CodeRabbit

  • Refactor
    • Improved strict mode detection and handling in switch statement execution for consistency and maintainability.

✏️ Tip: You can customize this high-level summary in your review settings.

@rogeralsing rogeralsing merged commit 7fe4d1f into main Jan 6, 2026
8 of 24 checks passed
@coderabbitai
Copy link

coderabbitai bot commented Jan 6, 2026

Caution

Review failed

The pull request is closed.

📝 Walkthrough

Walkthrough

The changes refactor strict mode handling for switch statements by introducing an IsStrict field to SwitchInstantiationPlan. Detection logic moves into SwitchStatement, which now scans cases for strict mode and passes the result to the plan. SwitchStatementExtensions then uses the plan's flag instead of receiving it as a parameter.

Changes

Cohort / File(s) Summary
Switch Instantiation Plan Refactoring
src/Asynkron.JsEngine/Ast/SwitchInstantiationPlan.cs, src/Asynkron.JsEngine/Ast/SwitchStatement.cs, src/Asynkron.JsEngine/Ast/SwitchStatementExtensions.cs
Added bool IsStrict field to SwitchInstantiationPlan primary constructor (first parameter). SwitchStatement now detects strict mode by scanning switch case bodies and passes this flag when constructing the plan. SwitchStatementExtensions updated to retrieve strict mode from the plan instead of accepting it as a method parameter, removing the bool isStrict parameter from InstantiateSwitchLexicalDeclarations.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 hops with glee
Switch plans now hold their strictness tight,
No more parameters passed left and right!
From cases scanned to plans designed,
Strict mode logic, neatly aligned! ✨

✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between acb51c8 and c0858fd.

📒 Files selected for processing (3)
  • src/Asynkron.JsEngine/Ast/SwitchInstantiationPlan.cs
  • src/Asynkron.JsEngine/Ast/SwitchStatement.cs
  • src/Asynkron.JsEngine/Ast/SwitchStatementExtensions.cs

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c0858fdb9e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +30 to +33
var switchEnv = new JsEnvironment(environment, false, instantiationPlan.IsStrict);

// Push a scope context for the switch block
var scopeMode = isStrict ? ScopeMode.Strict : ScopeMode.Sloppy;
var scopeMode = instantiationPlan.IsStrict ? ScopeMode.Strict : ScopeMode.Sloppy;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Avoid promoting strict mode to all switch cases

Because instantiationPlan.IsStrict is derived from any case body’s IsStrict, the single switch environment becomes strict for all cases as soon as one case starts with a "use strict" directive. In sloppy code this changes semantics for other cases (e.g., a non‑strict case assigning to a read‑only property or deleting a non‑configurable property will now throw), even if the strict case is never matched. Strictness should follow the outer scope or be applied per‑case, not promoted to the entire switch.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants