[BinAST] Parsing Error: Variable redeclaration

RESOLVED FIXED in Firefox 65

Status

()

defect
P3
normal
RESOLVED FIXED
9 months ago
8 months ago

People

(Reporter: arai, Assigned: arai)

Tracking

Trunk
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 disabled, firefox65 fixed)

Details

Attachments

(2 attachments)

Assignee

Description

9 months ago
Posted file redecl.binjs
encoded from
https://searchfox.org/mozilla-central/source/js/src/jit-test/tests/basic/bug714614.js

tested on m-c 6f8701d1be0c + patches for other bug fixes under bug 1495611

Configure flags: --enable-warnings-as-errors --disable-optimize --enable-debug

Runtime flag: -B redecl.binjs

Actual result:
SyntaxError: BinAST Parsing Error: Variable redeclaration
OK, so the problem here is that we have function(foo, foo), and we see two AssertedParameterNames, one or each.

arai, how does this interface with your other parameter work? Should we change the encoder to only emit 1 binding in this case, or just not throw?

I think I'm in favor of having only unique bindings names in the AssertedParameterScope, since we want capture information to be unambiguous.
Flags: needinfo?(arai.unmht)
Assignee

Comment 2

9 months ago
(In reply to Eric Faust [:efaust] from comment #1)
> OK, so the problem here is that we have function(foo, foo), and we see two
> AssertedParameterNames, one or each.
> 
> arai, how does this interface with your other parameter work? Should we
> change the encoder to only emit 1 binding in this case, or just not throw?
> 
> I think I'm in favor of having only unique bindings names in the
> AssertedParameterScope, since we want capture information to be unambiguous.

yeah, including unique binding name makes sense,
but in that case AssertedPositionalParameterName and CheckPositionalParameterIndices don't fit, so we'll need to tweak them.
anyway, I'll bring this to spec issue.
Flags: needinfo?(arai.unmht)
Assignee

Updated

9 months ago
Flags: needinfo?(arai.unmht)
Assignee

Updated

9 months ago
Blocks: 1498488
Assignee

Updated

9 months ago
No longer blocks: 1498488

Comment 3

9 months ago
Positional parameter names should allow duplicates in certain modes, because those names are meant to be taken as ordinal in their position in the list.
Assignee

Comment 4

9 months ago
(In reply to Shu-yu Guo [:shu] from comment #3)
> Positional parameter names should allow duplicates in certain modes, because
> those names are meant to be taken as ordinal in their position in the list.

I'm not sure if I understand what you mean,
but, the items in AssertedParameterScope.paramNames are supposed to represent the binding information,
and only the last item of duplicate parameters is the actual binding.
so, IMO, there should at least be some difference between the last one and others.
(of course, we can detect such case in implementation side and handle the last one differently, with current spec, tho)
Assignee

Updated

8 months ago
Flags: needinfo?(arai.unmht)

Comment 5

8 months ago
Ah, please ignore my comment. I had forgotten that AssertedPositionalParameter included the index explicitly. Sorry!
Assignee

Comment 6

8 months ago
ni myself to file spec issue tomorrow
Flags: needinfo?(arai.unmht)
Assignee

Comment 8

8 months ago
In case we use the current spec without modification, here's the code that just allows the duplicate entry (only in non-strict mode)
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
Attachment #9020264 - Flags: review?(efaustbmo)
Comment on attachment 9020264 [details] [diff] [review]
Allow duplicate AssertedPositionalParameterName.

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

lgtm. Do we need to do more for parameter checks, or is just not throwing going to suffice?
Attachment #9020264 - Flags: review?(efaustbmo) → review+
Assignee

Comment 10

8 months ago
Thank you for reviewing!

(In reply to Eric Faust [:efaust] from comment #9)
> Do we need to do more for parameter checks, or is just not throwing
> going to suffice?

not throwing is fine.
remaining parts is handled in emitter side.

Comment 12

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/978ef31db350
Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.