bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Separate dynamic module scopes from those of function calls

RESOLVED FIXED in Firefox 44

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

(Blocks: 1 bug)

unspecified
mozilla44
Points:
---

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
Created attachment 8678224 [details] [diff] [review]
disentangle-scope-object

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)

Updated

3 years ago
See Also: → bug 1193490

Comment 1

3 years ago
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+
(Assignee)

Comment 2

3 years ago
Created attachment 8679464 [details] [diff] [review]
disentangle-scope-object v2

(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)
(Assignee)

Updated

3 years ago
Attachment #8678224 - Attachment is obsolete: true

Updated

3 years ago
Attachment #8679464 - Flags: review?(shu) → review+

Comment 4

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/58d4fc528b3b
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.