-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Swap priority order of bools/strings if strictBooleans is on #6044
Changes from 3 commits
93cd2a3
79ece8f
d3e5811
6637ca7
2c3619f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ import { maxValue, minValue, places } from "./utils"; | |
export function isMoreSpecificMultiple( | ||
types1: Format.Types.OptionallyNamedType[], | ||
types2: Format.Types.OptionallyNamedType[], | ||
strictBooleans: boolean, | ||
userDefinedTypes: Format.Types.TypesById | ||
): boolean { | ||
//just wrap the types in tuples and defer to isMoreSpecific() | ||
|
@@ -21,7 +22,13 @@ export function isMoreSpecificMultiple( | |
typeClass: "tuple", | ||
memberTypes: types2 | ||
}; | ||
return isMoreSpecific(combinedType1, combinedType2, userDefinedTypes, true); | ||
return isMoreSpecific( | ||
combinedType1, | ||
combinedType2, | ||
strictBooleans, | ||
userDefinedTypes, | ||
true | ||
); | ||
//that last flag is so we ignore variable names at top level | ||
} | ||
|
||
|
@@ -30,6 +37,7 @@ export function isMoreSpecificMultiple( | |
export function isMoreSpecific( | ||
type1: Format.Types.Type, | ||
type2: Format.Types.Type, | ||
strictBooleans: boolean, | ||
userDefinedTypes: Format.Types.TypesById, | ||
ignoreComponentNames: boolean = false //this flag is *not* applied recursively! | ||
): boolean { | ||
|
@@ -40,7 +48,7 @@ export function isMoreSpecific( | |
if (type2.typeClass === "userDefinedValueType") { | ||
type2 = getUnderlyingType(type2, userDefinedTypes); | ||
} | ||
const typeClasses = [ | ||
let typeClasses = [ | ||
["options"], | ||
["array"], | ||
["struct", "tuple"], | ||
|
@@ -52,6 +60,13 @@ export function isMoreSpecific( | |
["string"], | ||
["bool"] | ||
]; | ||
if (strictBooleans) { | ||
//swap priority of strings and bools | ||
typeClasses.pop(); | ||
typeClasses.pop(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks brittle; can we do it another way? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, there's any number of alternative ways to do it, the question is what's good without being too repetitive. I guess we could have everything but the last two, then tack on the last two depending on the flag? Would that be less brittle? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (I've made this change, let me know if you think a different way would be better.) |
||
typeClasses.push(["bool"]); | ||
typeClasses.push(["string"]); | ||
} | ||
//for each type, what's the first one it counts as? | ||
const index1 = typeClasses.findIndex(classes => | ||
classes.includes(type1.typeClass) | ||
|
@@ -78,12 +93,14 @@ export function isMoreSpecific( | |
//we haven't actually checked visibility, so we'll have to coerce | ||
<Format.Types.FunctionExternalType>type1, | ||
<Format.Types.FunctionExternalType>type2, | ||
strictBooleans, | ||
userDefinedTypes | ||
); | ||
case "array": | ||
return isMoreSpecificArray( | ||
type1, | ||
<Format.Types.ArrayType>type2, | ||
strictBooleans, | ||
userDefinedTypes | ||
); | ||
case "bytes": | ||
|
@@ -102,6 +119,7 @@ export function isMoreSpecific( | |
return isMoreSpecificTuple( | ||
type1, | ||
<Format.Types.TupleType>type2, | ||
strictBooleans, | ||
userDefinedTypes, | ||
ignoreComponentNames | ||
); | ||
|
@@ -192,6 +210,7 @@ function isMoreSpecificString( | |
function isMoreSpecificArray( | ||
type1: Format.Types.ArrayType, | ||
type2: Format.Types.ArrayType, | ||
strictBooleans: boolean, | ||
userDefinedTypes: Format.Types.TypesById | ||
): boolean { | ||
//static is more specific than dynamic, but | ||
|
@@ -205,13 +224,19 @@ function isMoreSpecificArray( | |
//length and types must both be more specific | ||
return ( | ||
moreSpecificLength && | ||
isMoreSpecific(type1.baseType, type2.baseType, userDefinedTypes) | ||
isMoreSpecific( | ||
type1.baseType, | ||
type2.baseType, | ||
strictBooleans, | ||
userDefinedTypes | ||
) | ||
); | ||
} | ||
|
||
function isMoreSpecificFunction( | ||
type1: Format.Types.FunctionExternalType, | ||
type2: Format.Types.FunctionExternalType, | ||
strictBooleans: boolean, | ||
userDefinedTypes?: Format.Types.TypesById | ||
): boolean { | ||
switch (type2.kind) { | ||
|
@@ -238,6 +263,7 @@ function isMoreSpecificFunction( | |
!isMoreSpecific( | ||
type1.outputParameterTypes[i], | ||
type2.outputParameterTypes[i], | ||
strictBooleans, | ||
userDefinedTypes | ||
) | ||
) { | ||
|
@@ -256,6 +282,7 @@ function isMoreSpecificFunction( | |
//swapped for contravariance, I guess...? | ||
type2.inputParameterTypes[i], | ||
type1.inputParameterTypes[i], | ||
strictBooleans, | ||
userDefinedTypes | ||
) | ||
) { | ||
|
@@ -282,31 +309,34 @@ function isMutabilityMoreSpecific( | |
function isMoreSpecificTuple( | ||
type1: TupleLikeType, | ||
type2: TupleLikeType, | ||
strictBooleans: boolean, | ||
userDefinedTypes: Format.Types.TypesById, | ||
ignoreComponentNames: boolean = false | ||
): boolean { | ||
debug("type1: %O", type1); | ||
debug("type2: %O", type2); | ||
const fullType1 = (<TupleLikeType>Format.Types.fullType( | ||
type1, | ||
userDefinedTypes | ||
)); | ||
const fullType2 = (<TupleLikeType>Format.Types.fullType( | ||
type2, | ||
userDefinedTypes | ||
)); | ||
const fullType1 = <TupleLikeType>( | ||
Format.Types.fullType(type1, userDefinedTypes) | ||
); | ||
const fullType2 = <TupleLikeType>( | ||
Format.Types.fullType(type2, userDefinedTypes) | ||
); | ||
const types1: Format.Types.Type[] = (<Format.Types.OptionallyNamedType[]>( | ||
fullType1.memberTypes)).map(member => member.type); | ||
fullType1.memberTypes | ||
)).map(member => member.type); | ||
const types2: Format.Types.Type[] = (<Format.Types.OptionallyNamedType[]>( | ||
fullType2.memberTypes)).map(member => member.type); | ||
fullType2.memberTypes | ||
)).map(member => member.type); | ||
//lengths must match | ||
if (types1.length !== types2.length) { | ||
return false; | ||
} | ||
//individual types must satisfy isMoreSpecific | ||
for (let i = 0; i < types1.length; i++) { | ||
//note we do *not* pass along the ignoreComponentNames flag | ||
if (!isMoreSpecific(types1[i], types2[i], userDefinedTypes)) { | ||
if ( | ||
!isMoreSpecific(types1[i], types2[i], strictBooleans, userDefinedTypes) | ||
) { | ||
return false; | ||
} | ||
} | ||
|
@@ -316,9 +346,11 @@ function isMoreSpecificTuple( | |
//(and all exist) | ||
//then compare by component names in addition to by position | ||
let names1: string[] = (<Format.Types.OptionallyNamedType[]>( | ||
fullType1.memberTypes)).map(member => member.name); | ||
fullType1.memberTypes | ||
)).map(member => member.name); | ||
let names2: string[] = (<Format.Types.OptionallyNamedType[]>( | ||
fullType2.memberTypes)).map(member => member.name); | ||
fullType2.memberTypes | ||
)).map(member => member.name); | ||
//we just created these via a map so it's OK to sort in-place | ||
names1.sort(); | ||
names2.sort(); | ||
|
@@ -331,7 +363,7 @@ function isMoreSpecificTuple( | |
} | ||
if (namesEqual) { | ||
debug("names equal"); | ||
for(let i = 0; i < types1.length; i++) { | ||
for (let i = 0; i < types1.length; i++) { | ||
const type1 = types1[i]; | ||
const name = fullType1.memberTypes[i].name; | ||
const type2 = fullType2.memberTypes.find( | ||
|
@@ -340,7 +372,7 @@ function isMoreSpecificTuple( | |
debug("name: %s", name); | ||
debug("type1: %O", type1); | ||
debug("type2: %O", type2); | ||
if (!isMoreSpecific(type1, type2, userDefinedTypes)) { | ||
if (!isMoreSpecific(type1, type2, strictBooleans, userDefinedTypes)) { | ||
debug("returning false"); | ||
return false; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be in an
options
style argumentThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be if you really want, but this is sufficiently internal that I'm not really worried about that sort of thing -- there isn't a need to maintain a stable interface here, we can just change it as needed. Adding yet another
options
here seems more confusing than just adding a flag.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really think we should always lean in favor of options-style arguments, but I won't press the issue here