-
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
Object.fromEntries ignores this / Symbol.species #2582
Comments
|
We're using builtin extend to actually secure some of our code that lands in user-land-minefield where there are evil scripts overriding class SecuredObject extends Object {
static get[Symbol.species]() { return SecuredObject; }
}
sealAsOwnPropertiesStatics(SecuredObject);
sealAsOwnPropertiesPrototype(SecuredObject.prototype); And then we discovered that |
The collection of static methods on |
In any case, this isn't the right forum for this discussion; to propose changes to the language, please open an issue on es-discourse, as mentioned in CONTRIBUTING.md. |
@bakkot I'll do that when I can, but I consider this a bug, not a feature request. |
It's not a bug - it was intentional, it matches web reality, and it does not violate any assertion or written invariant. It's an inconsistency, at best, and whether it is in fact an inconsistency is a matter of perspective. |
Fair enough, but I consider inconsistencies more like bugs, so there is where we can agree to disagree, and the reason I opened this here instead. Anyway, point taken, I'll raise this in the proper channel, thanks for your patience 👍 |
See this explanation from @allenwb , and specifically:
With EDIT: Specifically, I’m saying that even if we consider that |
@claudepache sure, I think @zloirock already mentioned that, and it was my bad ... however, this is all I am after:
precisely that ... with |
For history/documentation sake, the class MyString extends String {}
new MyString(" abc ").trim() instanceof MyString;
// false
So, how big of an argument is that Object is not meant to be used as class when:
These are all the reason I've opened an issue here: this is a bug to me, nothing more, nothing less. |
Yes, because Object is special, and isn’t meant to be subclassed in that manner. Object.getOwnPropertyDescriptor doesn’t look at the receiver either. As was said above, Object is a namespace for its static methods; they’re not part of a class. |
Put another way, if Object worked this way, then Boolean.fromEntries and Function.fromEntries would work as well, because proper class inheritance means the constructor inherits too. They don’t, though, because Object is special. |
Put another way as well: Object.prototype.typeof = function () {'use strict';
return typeof this;
};
(true).typeof();
// "boolean" I understand the special part of Object is the fact nothing inherits from its static properties, which is OK, core speaking, but the moment one can extend it, and all statics are inherited, I don't understand what's so special about the only method that shouldn't fail expectations which is I actually wouldn't be here if:
|
TBC, I'd prefer to have something like this: NullProtoObject.getOwnPropertyDescriptor(...) instanceof Object; // => false |
I don't think this came up at all when it was a proposal - probably because everyone was already just on the same page with how @ljharb described it above.* It was intended as a utility for "generic" object transformations with no more specific relationship to Object-as-constructor than Object.entries (which returns an array of arrays, but has no
Is shadowing fromEntries in the subclass not acceptable here? class SpecialObject extends Object {
static fromEntries(iterable) {
return super.setPrototypeOf(super.fromEntries(iterable), this.prototype);`
}
// or maybe, if the subclass constructor here does its own stuff or adds a brand?:
static fromEntries(iterable) {
return new this(super.fromEntries(iterable));
}
} Like other plain-object returning ops, it doesn't actually invoke the/an Object constructor. With no constructor in the mix, I don't think it makes sense to say a constructor's species is not being honored. * There was however a discussion about specifying a prototype argument iirc. Although that wasn't where things landed, I think that would make more sense than @@species here, since the other "make a new object" static method, |
FWIW, the Object variants, despite their similar names and behaviors, remain useful. The Reflect variants specifically implement the Proxy handler contract - for example, they return false where the Object variants would throw (which is what you'd usually want - a Proxy will ultimately throw for most |
my question is: why is that needed in the first place ??? there's no rationale to explain why methods are inherited but methods that construct resulting instances (just one, fromEntries) shouldn't consider Moreover, the reason we realized this footgun is that we secured classes so that every edit also worth mentioning this was literally the first post:
yes, we measured performance too .. it's 2 to 10 times slower on average, even pre-securing |
Note that (0, ArraySubclass.from)(whatever) returns an instance of (0, Promise.resolve)(whatever) which throws, or So if your goal here is that your subclass should behave just like the superclass except using its methods will produce instances of your subclass instead, you'll need to explicitly override Short of auto-binding getters, this tradeoff is inherent: either you can't use the "static method" as a function, or you can but then it doesn't do what it looks like if you pull it off a subclass instead of the base class. Both choices have significant downsides. |
perfect, makes it consistent with Object too, pretty please? if you justify Should it fall back to |
if anything left to say: please find any piece of code that would break in the world if |
I think, as others mentioned, subclassing Object being possible doesn’t imply it’s something the language has aimed to facilitate or privilege beyond “Object should do something rational when invoked with a I can try to enumerate the reasons why I think* Object subclassing is a technically-possible thing rather than a pattern facilitated or encouraged by the language. It won’t be anything you don’t already know I’m sure, but maybe seeing it laid out will be helpful anyway (e.g. so that you can pinpoint which aspects of these lines of thought you don’t think are sound.) (This list isn’t meant to be like ... nailing up theses or anything. It’s my best understanding of why-things-are-as-they-are and some why-that's-reasonables, but the most subjective items are indeed subjective and not “correct”.)
Affordances specifically meant to improve ergonomics of subclassing Object seem unlikely to get a buy-in given the above — but affordances that do so and also place a performance tax on non-subclass usage, even a very minor tax, seem particularly unlikely to find sympathies. All that said, the case you’ve described (realm-poisoning paranoia) is very interesting and I’m curious to hear more about it — I’m not entirely sure why it’s led you down this particular path. As a ridiculously obsessed poisoning nut myself, my toolbox is full to the brim with
|
|
All of these issues aside, the only change that could likely be made would be to make it like In order to figure that out one way or the other, a browser would have to be willing to invest the time and effort into gathering that data in the first place, which they've historically been very hesitant to do even about much broader proposals like In order for a browser to be willing to do that in the first place, the change would almost certainly first have to gain consensus, which would mean every delegate would have to deem the change neutral or desirable - and there's a number of delegates in this thread that have already indicated they would find it undesirable. This would have been a great item to bring up during the proposal process itself - ideally before stage 3 - whether it changed the outcome or not, but at this point, I don't see what more there is to discuss. |
Andrea, I like you very much. I hope you can see that my attempt at answering the question was intended in good faith even if the lack of complete agreement here has been frustrating. :) |
By example (or last resort): class MyObject extends Object {}
(new MyObject) instanceof MyObject;
// true
(new MyObject({a: 123})) instanceof MyObject;
// true
MyObject.fromEntries([]) instanceof MyObject;
// false There is no rationale there, and that's the bug I've filed to TC39. Thanks for considering fixing that. |
@WebReflection anyway, TC39 wants to remove built-ins subclassing https://github.com/tc39/proposal-rm-builtin-subclassing. Subclassing support already removed from new proposals like grouping or changing array by copy, so adding it here could be strange. They don't care that remove built-ins subclassing will break half of the web since even now they discuss it and they don't understand what and where will be broken, despite the fact that I showed enough examples. |
this is the gotcha I've found around these topics ... I know @ljharb was strong about this current spec'd behavior, and yet these sentences are worth zero without data behind. No, I bet the other way ... I like having JS being bet driven programming language though, but I think TC39 members are influencing too much their own bets in there ... edit I am pretty sure when Brendan mentioned "always bet on JS" he didn't really mean this kind of development ... |
@WebReflection and you might be right that it's a viable change! but even if it was unanimously desirable, which it is currently not, the obstacles I mentioned still exist. |
that was my TIL, and we had a laugh on twitter already ... JS is like "we can't break the Web from 90's" but full steam ahead when it comes to "we can drop recent APIs from 2015 on because YOLO" ... it's bad for the language, and bad for the standard, in terms of reliability, or "betting", for the next cool thing ... way too many cool things have been regretted in last 10 years, and so many bad things still there 'cause "we can't break the Web" ... sad progress, imho, but also not the right venue to discuss this. I'll show myself out this thread, but thank you all for participating, it's been surely interesting |
I don't think making it work the way you suggest would break anything. I just don't think it's a good idea, because it only makes sense if you think of Separately: in my experience, the overwhelming majority of object-like things which need a different prototype than |
@bakkot I am OK about const myObj = new MyObj({...whatever});
if (myObj.hasOwnProperty(mySecret)) {
// ... we really want to trust this execution
} So that's it: TC39 can remove subclassing in the name of security, and contextually enable the most insecure environment ever, because everyone can pollute globals, and not just ASAP, but literally whenever they land/want to, so last come, last shenanigans happens. This is not where JS should go, and I have no idea why TC39 is pushing so hard to make it happen by removing builtin extends, either on the specs or on the DOM (but that's another story). |
@WebReflection I doubt complete @@species removal will end up happening. As zloirock said, it's crystal clear that it will wreak chaos and on a more meta level might be pretty devastating to the faith people have in the standard. But IIUC (maybe?), the folks investigating that are using that as a kind of "this would be ideal from our POV" starting point from which to discover where the line really is. Some of the more overwrought interior hooks might end up safe to cut, and there's no harm in exploring whether that's possible and if so, what that would look like. In any case, I don't think it's the same as "removing builtin extends" - it concerns (potentially) removing complex "interior" contract stuff, but those hooks don't really achieve things that can't already be achieved by other means. Some of those hooks, e.g. the current RegExp flag getter behaviors, are arguably kinda silly. I think it's notable tho that "Object.fromEntries shouldn't be receiver-sensitive" is kinda unrelated? For one, it never was receiver sensitive (nothing's going away), but more importantly, the reason it's not receiver-sensitive isn't related to objections to interior contract hook stuff . They just stem from a different conception of what these methods "are" (object utility functions sitting on a namespace object). |
I'm very familiar with proto poisoning attacks, yes, and understand the goal of writing code which is defensive against them. But if your goal is to replace let { fromEntries } = Object;
let obj = fromEntries(whatever); and even with the change you propose, switching out
As the readme of that proposal says, the primary motivation is that extending builtins is extremely complex for engines, such that it has been one of the more common sources of browser exploits (not just application security issues) across all browsers since its introduction, without providing a level of benefit to users which is even remotely commensurate with that level of complexity and risk. I am not at all confident it will prove possible to remove wholesale, but that's the motivation. And I'm hopeful we can at least get rid of some of the worst excesses, like subclassing typed arrays and RegExps. |
hisses you shall never take my precious lexer/parser toolkit of TypedArray subclasses! ~ UTF8Array, SourceText, SourceFrament ... (I hope. But I will take my own medicine if I have to :) - everything in my TypedArray menagerie already has to do some extra stuff to choose to return either the base type or the subclass since @@species actually isn't expressive enough anyway, e.g. returning Uint32Array for slice, but SourceText for subarray. Having to shadow subarray etc won't really end my world ...) |
sure thing, and that's because the spec is broken? hence we're here!
because the issue is with
I don't understand this sentence at all, because nobody in the industry extends builtins in general, except people, or library, that know what they are doing. When they do, is because they .. actually, likely, know, what they are doing.
And they know that too, right?
and I am sure whoever is doing that has a reason to do so, and we securing RegExp have a reason too ... for gosh sake, make the most powerful string manipulation tool impossible to secure and goodbye JS ?!? |
(I think the complexity/risk in question is that of the JS engine's implementation, not the code using the features.) |
RegExp is currently the most challenging intrinsic (well, apart from Promise) to make robust in an env where you cannot just freeze or grab from a "private" realm. The reason it's so challenging is the hooks that they're trying to remove, i.e. that would actually become much easier, not harder / impossible. |
We're securing RegExp through a class extend with sealed/frozen methods and statics ... honestly folks, if you ever want to know how dirty the game could be, please join my team, otherwise, please never tell me again not extending builtins is a secure measurement, because that's really not what it is: JS is exploitable from |
OK, so it sounds like it's not that you specifically want If so, you could suggest a proposal for, say,
Browser exploits are an entirely different class of security vulnerability. Application-level security measures aren't going to do anything if the adversary is capable of getting an RCE in the browser itself, as has happened several times as a consequence of the complexity inherent in trying to make extending builtins work with all the machinery that is currently specified. |
How do you think to get rid of such worst excesses? |
like |
Since it’s not what everyone expects, and is different from what some expect? Yes, it’s a huge change. |
It would not be a huge change in terms of the semantics of the language. The problem is that many people, myself included, do not share your expectation of how it would work. |
Accordingly with your thoughts, nobody does that (as in, Object shouldn't be a class extend and so on) ... so how is nobody doing that expecting anything, and how is that somebody doing that found it a surprise? |
anyway ... my thoughts right now: emptiness on one side, real-world uses cases on the other this is where I am here, or elsewhere, if "Object is special because" or "can't extend built-ins because" 😢 |
It might be good to return to the motivating problem a bit (as I understand it)? The merits of and objections to a specific potential solution are fascinating, but there are likely other angles entirely. Bakkot suggested |
Every time someone proposes an extension to any API it's because they have a use case for it. We're not going to add all possible such extensions. So we're always going to have to talk about whether the proposed design makes sense in the context of the rest of the language. That's not emptiness. I recognize that you and I disagree about which design makes more sense here; there's never going to be an objectively right answer to that. But, as I said above, it also seems to me that there are other possible things which would give you what you want, such as a possible |
what I really expect is something as simple as this: // take current fromEntries and setPrototypeOf
const {fromEntries, setPrototypeOf} = Object;
const apply = Function.call.bind(Function.apply);
// but actually use fromEntries with a better meaning
Object.fromEntries = function () {
return setPrototypeOf(
apply(fromEntries, this, arguments),
this.prototype
);
};
// ... so that ...
class Thing extends Object {}
Thing.fromEntries([]) instanceof Thing;
// true what I do think it should happen, is that the instance is created and populated based on Object.fromEntries = function fromEntries(entries) {
'use strict';
const ref = new (this || Object);
for (const [key, value] of entries)
ref[key] = value;
return ref;
}; |
It'd have to use [[Define]] and not [[Set]], of course, or it'd break a bunch of otherwise robust code, but I'm also not sure why this change - which would likely make things slower for everyone who wasn't subclassing Object - is better than you adding this to your subclass: static fromEntries(entries) {
return new this(super.fromEntries(entries));
} |
because it doesn't reason with the fact nobody needs to do that with where is the slowness there? or why isn't |
P.S. I've already said |
Sure, but if that’s a concern then you’d want with your own proposal to still do |
need to check if |
Description:
The following code works:
while the following code doesn't:
Even after setting explicitly species:
I find this inconsistency very surprise prone, and I think this is a specification bug, because we can brand our own Arrays but we cannot brand our own Objects unless we use
Object.setPrototypeOf(Object.fromEntries(arr), MyObject.prototype)
which is verbose, ugly, and also slow.Thank you for considering updating specs around this behavior.
The text was updated successfully, but these errors were encountered: