-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Destructuring declarations that bind nothing should probably be an early error #97
Comments
On the surface fixing this bug seems like something we could get away with while there are no interoperable destructuring implementations. Nice find! |
Oops, just sent an email to esdiscuss before I saw this. Yay! :) |
@nzakas thanks much for pointing this out :) Definitely file bugs for any other issues you find! |
@allenwb what about non-binding forms (AssignmentPattern and friends)? Should |
I also note that |
I have strong concerns about this proposal:
It sounds nice to assume that Since @nzakas has already landed a linter rule for eslint for catching these things, I think that is the better path. Let tooling catch/warn about these, but don't make the language prevent these potentially not-mistake cases. |
I disagree with this proposal. It creates an irregularity and makes life harder for code generators, but also for people who want to comment out parts of a pattern. I don't think there is any real win either. Moreover, as Brian pointed out, destructuring without a RHS is an error already. |
I think Another scary pattern is this: let { loc: [] } = foo; @rossberg-chromium this isn't just about variable declarations, there are also assignments. You'd still be able to comment out parts of a pattern, just not every part at once. You're also missing this use case completely: let { foo: {} } = bar; @getify I'm having a hard time understanding why commenting out every property at once is useful for debugging. Isn't this: var {
// x,
// y
} = foo(); The same as: // var {
// x,
// y
// } = foo(); ? |
The RHS is evaluated before the binding pattern is walked, so |
@caitp yes, but unless you're relying on side effects, that difference is inconsequential. |
Assuming goes both ways. :) In any event, inability to comment out code just the way you want it doesn't seem like it should be a blocker for this. We all have editors, most have shortcut keys for inserting comments. If you need to use two keystrokes instead of one to help prevent unnecessary errors that are hard to find, that seems like a good tradeoff (IMHO). You can always move the RHS of an assignment to the next line temporarily and get the same effect. |
intentional side effects sometimes are as important but yet benign as console.log statements inside |
There's always... /*var {
// x,
y
} =*/ foo(); Clarifying edit: I don't feel strongly about this, but I also don't think that changes to the ability to single-line-comment at dev time for some rare set of cases is all that compelling a reason not to do this. I mean if this is really an issue, what do you do when some code does |
yes of course you can. but my point is I shouldn't have to. you make the one-sided assumption that my case ought to require me to type/edit more. i'll reiterate my similarly one-sided assumption/assertion from earlier: if you want protection from this "mistake" in your code, you should have to use a smart tool to protect you. but don't force that assumption on the rest of us. |
FWIW, I agree with @getify and @rossberg-chromium for the same reasons they've stated. Please don't take this as dog-piling, as I will gladly argue against this at the next meeting ;) |
Can someone give a little more detail about how removing a binding is useful for debugging purposes? It seems unlikely that removing a binding would have an interesting effect on a program. Also, it's clear this needs consensus! |
@bterlson, less about the specific thing you might do and more about the strange error you'll encounter when doing perfectly reasonable things during development/debugging. In fact, you might want to eliminate all bindings to observe the outcome—it doesn't really matter why, it's just something a developer might do, and I think the language doesn't need to prohibit it just because it can. This is the sort of thing that dev tools (i.e. linters) can handle—if the end developer wants to. |
in one of my cases, the properties I was destructuring were getters. the side effects were noisy console.log statements that I wanted to avoid by simply commenting out the assignment item in the pattern. |
When commenting out every destructuring property in an object destructuring, could you not just add a nonexistent garbage property, and still do your debugging without having to worry about whether "no destructuring patterns" are an error? I think the real question here is, which is going to be more common: that an empty-bindings destructuring pattern is a developer mistake (worth noting that things like the TDZ attempt to address developer mistakes, and I think this is indeed the language's job), or, that an empty-bindings destructuring pattern is a temporary thing that an educated developer is doing for debugging? I tend to believe that both the former case is more common, and that in the latter case, the developer will not have a practical problem avoiding this error during debugging. @rwaldron @getify @rossberg-chromium, do you really think both that the former (a newbie developer making a mistake) is far less common than a developer needing to debug destructuring with things that have side effects such that they need to comment out every destructuring binding, and that discontinuing to allow the latter would be excessively burdensome on an experienced developer during their debugging? |
@rwaldron I'm not arguing this should be prohibited just because it's possible to prohibit. I see it as a mistake people might appreciate an error for like omitting other BindingIdentifiers. The error seems especially hard to spot in nested forms, and with initializers it gets harder still. But if this isn't a valuable error then I agree no reason to throw it. |
that's not really the question, IMO. is it useful? it seems likely it might be useful to some people in some cases. but it's also something that clearly it's something that linting tools can easily check for, giving developers an opt-in for that checking, rather than locking developers into it by language design. I think adding a lock-out to the language design deserves a higher bar than "useful/valuable error to some" (or even "many"). |
@ljharb i don't think it's fruitful to have a guessing-match at who's got the corner on more common. it's clear what you and nicholas think, but i don't agree. shrugs. however, no one seems to be addressing my second use-case above... the so i guess my only counter-argument is that this proposal has only presented one kind of use-case where the mistake happens (nested destructuring where they typed |
You can use linting tools to find errors, but I believe some errors are helpful enough to throw anyway. Regardless, I am not strongly in favor of making this an error (we allow many empty braces in various places already). I can see the rationale for keeping the current semantics. |
Similar to @getify's |
Sure, but why require such extra tedium?
Aside: TDZ is not just to prevent mistake, but to have meaningful behaviour In most other places, JavaScript consciously does not help the programmer |
So yea, the fact that empty destructuring itself can incur side effects (i.e. draining an iterator) now seems to me like a compelling reason to not do this. |
I somewhat contest that the assignment case should allow no identifiers if the binding patterns don't. It's certainly not clear that this is the "proper" way to do it. Wouldn't the same arguments would apply to making the empty assignment forms an error as well. |
What if I'm only triggering getters by 'use strict';
let foo = {
get bar() {
console.log('...');
}
}
let { bar: {} } = foo; I agree that's a horrible code, just saying it can be valid... |
Loose the { bar: {} } = foo; |
I'm not sure I understand the distinction between declarations and assignments, the error case is the same in both. In the former, the intent is to create new bindings but you fail to do so, in the latter, the intent is to assign a value and you fail to do so. And what about destructured function arguments? It seems like you can be bitten by this in numerous places, and I'm not sure I can rationalize why you'd get a safety net for one place but not others. |
@allenwb of course, that doesn't just work as cited, because then you'll have to remember to put the ( ) around the whole assignment... ...which is not only unnecessary but also invalid when the 'let' is present. |
@getify oops, of course. 0,{ bar: {} } = foo; |
The big difference I see is that declarations have non-local effect upon the static meaning or validity of parts of a program. For example, a declaration can shadow a declaration that exists in an outer scope and reference in an inner scope. A declaration can also make other declaration invalid (duplicate names). An assignment pattern does not have such non-local static effects. In my proposal at the top of this thread, I included a non-bindings check for destructuring function parameters. |
@allenwb, you can interpret the argument that some of us have made as: this change is potentially detrimental even to your criterion 1 ("All other things being equal, language should help humans write correct code."). I mentioned refactoring and principle of least surprise as reasons. |
@allenwb of your guiding principle is writing correct code, the local vs. nonlocal effects should be secondary to the fact that the assignment didn't actually assign, shouldn't it? @rossberg-chromium you're arguing that principle of least surprise says |
@nzakas, no, I don't see what's surprising about that (except for somebody who doesn't know destructuring). Destructuring binds 0..N variables, or assigns 0..N mutables. A needless discontinuity between 0 and 1..N is surprising (and annoying). |
I feel like many developers see destructuring assignment as simply sugar for assigning to variables - |
I too am curious if @rossberg-chromium would argue for making the binding identifier of let decls optional. It would allow binding 0..N variables and let you comment out a binding without commenting out its side-effecting initializer. Can leave it to linters to give you an error when you type |
@bterlson That's already a valid program. |
Riiight, although you'd likely get an error if let isn't declared. Still curious though - is that ambiguity the only reason the binding identifier isn't optional? |
If not for that ambiguity, I would be in favour of that proposal. |
I'm convinced. If I'm not declaring anything I should be warned that |
I dunno if this is a guiding principle of JS's design or not, but I believe it should be: any desired feature (or in this case, error) should be implemented in the least-burdensome way to those it would adversely affect -- adverse effects should be minimized. The proposal(s) here seek to shut out valid cases of usage by telling affected developers "just do something different". however, the main concern of this thread (mistake avoidance) could be handled in a less burdensome way by letting tools handle the checks. It's basically zero-cost to use tools to warn on this mistake for developers that want it, since such support has already landed. By contrast, it's a non-zero cost to shift the burden to developers to find some other way of doing any of the use-cases mentioned earlier. |
@getify obviously adverse effects and burdens on developers should be minimized. No one disagrees with that. |
@getify the difference is that you're claiming error avoidance is the job of tools, this proposal says error avoidance is the job of the language. I'm also in the latter camp, as you cannot expect everyone to be using tools to flag all errors nor can you assume all tools designed to find errors will find all bad patterns. |
@nzakas I am not broadly decrying all error avoidance in language design. There's an awful lot of good errors that JS asserts. This particular error avoidance doesn't just avoid errors (though you clearly see it only as errors). In a place where there's a tension between error avoidance and valid uses, I think that's a space where an argument can be made that tools can and should fill the gap, since tools are opt-in and configurable; language design is (largely) not. |
Empty binding has no problem in semantics but just problematic when human write JS. How do you think about JS code generated by programs? If empty binding is not allowed, I would have to make more effort to eliminate empty binding. Empty code and dead code may be problematic, but I'd like empty or dead code detection to be optional rather than required. |
@mandel59 see my comment above |
Though I agree with @getify and others that enforcing this proposal feels like the job of a linter and not the language itself, I'm worried that the current proposal will miss a large class of realistic mistakes, regardless of whether it is implemented by a linter or by the language. The most common mistake of this kind that I've encountered in real code is not "declarations that bind nothing," but confusion over the proper syntax for default values. A programmer who doesn't yet understand every detail of destructuring syntax could reasonably think that this code extracts the let { x, y: {} } = f(); Of course what the programmer really wants is let { x, y = {} } = f(); and there is real value in warning about the first pattern. However, if I understand this proposal, because the declaration at least binds |
As long as we're discussing potential mistakes that could potentially be caught "early" by tooling (or... ugh... the language), I personally think these are other common mistakes around destructuring: let { x } = ..
let [ y ] = .. Why? Because object destructuring and array destructuring don't have a guard built into them -- which I think they should have had! -- to handle when the r-value cannot be destructured. There's some cases that could be spotted easily, like: let { x } = null;
let [ y ] = undefined; But obviously the r-value is not always statically knowable, like But in the nested destructuring case, there is. Consider: let { x: { y } } = ..
let [ [ z ] ] = .. In these cases, if in their respective r-values, let { x: { y } = {} } = ..
let [ [ z ] = [] ] = .. The default value of |
@getify Though you've phrased that as an additional proposal, I get the feeling what you really mean is that we shouldn't get started down the path of adding warnings to the language as we think of them, because there are lots of corner cases that might potentially indicate programming mistakes, and we don't have a good theoretical framework for capturing all of them a priori. It's fine for linters to accumulate rules in response to common errors programmers actually make, but it's inappropriate for the language to enforce an incomplete set of noisy warnings. To make this argument more concrete, imagine specifying an error message for @getify's strawman rule about default values being required for nested objects/arrays. Can you really hope to word such a nuanced warning in a way that non-expert programmers can easily understand it? If not, the supposed motivation of helping the programmer avoid mistakes is greatly diminished. A linter, on the other hand, could link to a wiki page on its project website for further explanation, and it's acceptable for a linter to be wrong sometimes, because you opted into using it. It can even read a configuration file to determine what rules you care about enforcing. The language specification itself has none of these luxuries. |
@benjamn -- +1 |
Closing this as there was not consensus for changing the existing semantics. Thanks everyone for all the input!! |
In the ES6 spec., there is no check that a destructuring let/const/var actually introduces any bindings.
For example:
is a syntax error, but (updated 10/22/15)
is valid syntax that does nothing (it doesn't add any new variable bindings).
There are reports that some ES6 developers are encountering bugs cause by complex destructing patterns that unintentionally don't introduce any bindings.
This seems like a design bug in ES6. It would be trivial to specify that this is an early syntax error.
In 13.3.1.1
LexicalBinding : BindingPattern Initializer
- It is a Syntax Error if the BoundNames of BindingPattern has no elements.
In 14.1.2
FormalParameter : * BindingElement*
- It is a Syntax Error if the BoundNames of BindingElement has no elements.
Note that this is a breaking change for any program that actually contains such empty binding patterns. So there is some risk to making this change.
The text was updated successfully, but these errors were encountered: