Closed Bug 1217919 Opened 4 years ago Closed 4 years ago

Separate dynamic module scopes from those of function calls

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: jonco, Assigned: jonco)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch disentangle-scope-object (obsolete) — Splinter Review
Originally I made ModuleEnvironementObject derive from CallObject because it shares many features with it.  However this meant that every ModuleEnvironementObject was also a CallObject according to is(), which was confusing and needed an extra check everywhere we tested whether something was a call scope.  There were probably many places this didn't happen but should have.

Instead we can make a common base class that both derive from which I think makes more sense.

I've called this 'LexicalScopeBase', but that's probably too broad - better suggestions welcome.
Attachment #8678224 - Flags: review?(shu)
See Also: → 1193490
Comment on attachment 8678224 [details] [diff] [review]
disentangle-scope-object

Review of attachment 8678224 [details] [diff] [review]:
-----------------------------------------------------------------

I don't see ModuleEnvironmentObject being made a subclass of LexicalScopeBase.

If no tests failed -- that worries me a bit. I'd like to see a 2nd version of the patch because of that + the DebuggerScope thing below.

::: js/src/vm/ScopeObject-inl.h
@@ +48,5 @@
>  }
>  
>  inline void
> +LexicalScopeBase::setAliasedVar(JSContext* cx, AliasedFormalIter fi, PropertyName* name,
> +                              const Value& v)

nit: fix indentation

@@ +58,5 @@
>  }
>  
>  inline void
> +LexicalScopeBase::setAliasedVarFromArguments(JSContext* cx, const Value& argsValue, jsid id,
> +                                           const Value& v)

nit: fix indentation

::: js/src/vm/ScopeObject.cpp
@@ +1620,5 @@
>              return true;
>          }
>  
>          /* Handle unaliased formals, vars, lets, and consts at function scope. */
> +        if (scope->is<LexicalScopeBase>() && !scope->as<CallObject>().isForEval()) {

This check is inconsistent and would fail when scope is a ModuleEnvironmentObject. Moreover the body of the conditional explicitly expects a CallObject. Should this entire if be generalized to LexicalScopeBase?

::: js/src/vm/ScopeObject.h
@@ +295,5 @@
> +    inline void initRemainingSlotsToUninitializedLexicals(uint32_t begin);
> +    inline void initAliasedLexicalsToThrowOnTouch(JSScript* script);
> +
> +  public:
> +    /* Get/set the aliased variable referred to by 'bi'. */

Existing: the parameter is named 'fi'
Attachment #8678224 - Flags: review?(shu) → review+
(In reply to Shu-yu Guo [:shu] from comment #1)
> I don't see ModuleEnvironmentObject being made a subclass of
> LexicalScopeBase.

Ugh, I left out the most import part of the patch.  Fixing this didn't cause any additional problems however.

> ::: js/src/vm/ScopeObject.cpp
> >          /* Handle unaliased formals, vars, lets, and consts at function scope. */
> > +        if (scope->is<LexicalScopeBase>() && !scope->as<CallObject>().isForEval()) {
> 
> This check is inconsistent and would fail when scope is a
> ModuleEnvironmentObject. 

Ah, yes.  At the moment we alias everything in a module scope though so I just changed this back to check for CallObject as it did previously.
Attachment #8679464 - Flags: review?(shu)
Attachment #8678224 - Attachment is obsolete: true
Attachment #8679464 - Flags: review?(shu) → review+
https://hg.mozilla.org/mozilla-central/rev/58d4fc528b3b
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.