Remove isString type methods
#6803
Conversation
src/passes/Precompute.cpp
Outdated
| @@ -829,7 +829,8 @@ struct Precompute | |||
| return true; | |||
| } | |||
| // We can emit a StringConst for a string constant. | |||
| if (type.isString()) { | |||
| if (type.isRef() && type.getHeapType().isMaybeShared(HeapType::string)) { | |||
| assert(!type.getHeapType().isShared() && "TODO: shared string support"); | |||
There was a problem hiding this comment.
Why assert here? Why should we not be returning true?
There was a problem hiding this comment.
This still returns true for unshared strings, just like it did before. The new assertion prevents us from falling through and returning false. We don't want to try precomputing shared strings because they are generally not supported, tested, or fuzzed, so reporting that we can correctly precompute them is almost certainly incorrect at the moment, and in fact we should never be attempting to precompute them at all.
There was a problem hiding this comment.
What needs to be done for shared strings? They are immutable so modifications are not an issue. What kind of bug are you thinking of when you say it would almost certainly be incorrect to allow shared ones here?
There was a problem hiding this comment.
I expect we would run into some other assertion elsewhere or one of the bugs where the code hasn't been updated to account for shared types yet. I know for a fact that if we let the fuzzer get its hands on a test for shared strings, it will start failing assertions.
There may not be any fundamental problem with supporting shared strings, but it's just not something I would expect to work yet.
There was a problem hiding this comment.
Makes sense, but wouldn't it be best then to remove this assertion and let breakage happen elsewhere, if there is any? Otherwise, if we leave it, it's just an assertion we need to remove later. Or are you thinking this would be a canary to verify that we never run on shared strings at all? If the latter then maybe add a comment,
// There is no problem to allow shared strings to run in this pass, but error on it
// because other places are not yet ready, and this happens to be a convenient
// place to check.
There was a problem hiding this comment.
It seems better to fail early if we suspect we're going to fail later anyway, but you're right that this is just another assertion to remove later. I'll remove it since it's probably not pulling its weight.
Since there is only one unshared string heap type, the `isString()` method did not do anything that could not be done as well with other, more fundamental methods. Although using `isString` saved characters compared with its replacement, that's not enough of a benefit to offset the advantages of reducing redundant API surface and being more explicit at callsites. This would be NFC, but I updated many call sites of `isString` to handle shared string types as well, or at least error more eagerly on them, which may have changed observable behavior.
| @@ -829,7 +829,7 @@ struct Precompute | |||
| return true; | |||
| } | |||
| // We can emit a StringConst for a string constant. | |||
| if (type.isString()) { | |||
| if (type.isRef() && type.getHeapType().isMaybeShared(HeapType::string)) { | |||
There was a problem hiding this comment.
This verbosity is a pretty big downside to being explicit, I think. How about making things more uniform by adding such shorthand for other types? I mean that alongside type.isString() we could have type.isI31() and so forth? I think that would make for a much nicer reading experience.
There was a problem hiding this comment.
The verbosity is unfortunate, but I think it's the right choice. With a method like isString(), it's unclear from just reading the use site whether it includes shared strings, whether it includes both nullable and non-nullable references, etc. If we add a new dimension like shareability in the future, we would want to update the shorthand methods to include it, but that update might not be correct for some users. When you have to spell it out explicitly, there's much less room for error.
There was a problem hiding this comment.
I agree the verbosity is less risky, but I also think there is a natural interpretation here: isString/isArray/etc. should refer to either shared or non-shared. That is just a consistent extension of how they handle another property of types, nullability: isString/isArray/etc. ignore nullability, and likewise it is natural for them to ignore shareability.
In other words, I don't think this has been confusing at all regarding nullability, so I am not worried about shareability.
PR ##6803 proposed removing Type::isString and HeapType::isString in favor of more explicit, verbose callsites. There was no consensus to make this change, but it was accidentally committed as part of #6804. Revert the accidental change, except for the useful, noncontroversial parts, such as fixing the `isString` implementation and a few other locations to correctly handle shared types.
PR ##6803 proposed removing Type::isString and HeapType::isString in favor of more explicit, verbose callsites. There was no consensus to make this change, but it was accidentally committed as part of #6804. Revert the accidental change, except for the useful, noncontroversial parts, such as fixing the `isString` implementation and a few other locations to correctly handle shared types.
Since there is only one unshared string heap type, the
isString()method did not do anything that could not be done as well with other,
more fundamental methods. Although using
isStringsaved characterscompared with its replacement, that's not enough of a benefit to offset
the advantages of reducing redundant API surface and being more explicit
at callsites.
This would be NFC, but I updated many call sites of
isStringto handleshared string types as well, or at least error more eagerly on them,
which may have changed observable behavior.