Closed Bug 1711715 Opened 3 years ago Closed 2 years ago

Improve Ergonomic Brand Check error messages

Categories

(Core :: JavaScript Engine, task, P3)

task

Tracking

()

RESOLVED FIXED
99 Branch
Tracking Status
firefox99 --- fixed

People

(Reporter: mgaudet, Assigned: bthrall, Mentored)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Looking at this error message, and I realize it's not great.

> class A { #x; h(o) { return !#x in o; }}}
SyntaxError: private names aren't valid in this context

We can probably do better.

Severity: -- → N/A
Priority: -- → P3

hi @(In reply to Matthew Gaudet (he/him) [:mgaudet] from comment #0)

Looking at this error message, and I realize it's not great.

> class A { #x; h(o) { return !#x in o; }}}
SyntaxError: private names aren't valid in this context

We can probably do better.

hi o/

I would like to work on this bug :)
Could you explain better why this message is not great? And..what is expected to be great? I mean, it could help me to decide what I can do.

Hi Alexandra,

You can totally take on this bug. Let’s start by talking a bit about why this is a confusing error message to me, and what I think would be better.

This example was written by me while I was testing something else out. Let’s be clear: This code is actually buggy, based on the language precedence rules: Think about if I had written:

!’x’ in o

This silently is a bug; because ! has higher precedence than in, what this is really is asking is (!’x’) in o, not what you might have hoped for !(‘x’ in o)

Now, private fields kindly emit an error, which is good,: but the message “private names aren’t valid in this context” isn’t super helpful to me; the question is “What context?”

So, we can use SearchFox to find where this message is emitted, which finds three places. My suspicion here is that this is actually being emitted in here, where we parse a unary expression.

It would be good to check that my suspicion is right; a good way to do that is to build the spider monkey shell, then write a simple test case, then edit that call site to have a printf or MOZ_CRASH to verify that this is in fact the code that issues that message.

How to change the message is hard. Maybe something along the lines of “Cannot apply unary operator to private name”? Which is more clear.

Before getting started, you'll want to

Assignee: nobody → alesilva241
Status: NEW → ASSIGNED
Assignee: alesilva241 → nobody
Status: ASSIGNED → NEW
Assignee: nobody → bthrall
Status: NEW → ASSIGNED

@mgaudet says we've shipped private fields for long enough that it's
never coming out.

The test could be as simple as 1 + #x in y, but we want an expression that
fails to parse because of only this one error.

Depends on D139118

Pushed by ystartsev@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1d6aaab87a63
Remove check for enabling private fields r=yulia,mgaudet
https://hg.mozilla.org/integration/autoland/rev/7573e83b4966
Clarify error message for invalid private name in unary expression r=yulia
https://hg.mozilla.org/integration/autoland/rev/fc7c9b4732f6
Clarify error message for invalid private name due to operator precedence r=yulia
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 99 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: