widlparser icon indicating copy to clipboard operation
widlparser copied to clipboard

Remove optionality from name on all reasonable constructs.

Open tabatkins opened this issue 3 years ago • 1 comments

Despite virtually all constructs having a name, name was still marked as Optional[str] everywhere. This PR narrows that to str wherever possible, so I don't have to do lots of None-checking or casting when grabbing the names of things. Some particular details:

  • gave an unnamed OperationRest the empty string as a name, so it doesn't need to be Optional.

    All the uses of OperationRest that can be unnamed just test if they're unnamed and substitute a more specific string, like '__stringifier__'; unfortunately methods, which are always named, don't have a more specific grammar construction, and so they get inconveniently lumped into here. The "am I unnamed" test just checks the name as a boolean, so empty string works great here.

  • Similarly gave ExtendedAttributeUnknown an empty string as a name; it's a funky corner case that would otherwise infect ExtendedAttribute with an optional name even tho every recognized form of ExtendedAttribute has a name, and to avoid that you'd need to either always guard or always typecheck with one of the more specific subtypes, neither of which are useful to make authors do.

    Could replace with a raise or something, tho, if desired.

    Similarly, gave it an empty string normal_name, tho normal_name is often optional on other constructs. (Might be fixable, just didn't dive into it.)

    (Also added an ExtendedAttributeType alias that's a union of the specific subtypes, so ExtendedAttribute can properly narrow its attribute property and thus get good type-checking whenever it references that property.)

  • Left the big general superclasses (Construct, Production, ComplexProduction) as having an optional name, as they don't have names and you really can't depend on things. Had to sprinkle a few casts around for things that can contain anything, like NamespaceMember or InterfaceMember.

  • Deleted ArgumentList's name attribute; it was very odd in the first place (returning the name of its first arg when it had exactly 1 arg, but None if it had 0 or 2+ args).

  • Explored fixing type_name(/s) as well (bunches of type-related classes that are either None despite having a reasonable type name or missing entirely), but I'm honestly not sure what it's meant to be used for in the first place - it doesn't look anything internal calls these. So I left them alone.

tabatkins avatar Jun 03 '22 21:06 tabatkins

For example, the current Optional[str] behavior means I have to scatter casts all over code like this:

if optStart is not None:
    prefixes = [t.cast(str, method.name)]
    if method.name == "constructor":
        prefixes.append(t.cast(str, method.parent.name))
    for i in range(optStart, len(method.arguments)):
        argText = ", ".join(t.cast(str, arg.name) for arg in list(method.arguments)[:i])
        for prefix in prefixes:
            texts.append(prefix + "(" + argText + ")")

texts.append(t.cast(str, method.normal_name))
if method.name == "constructor":
    texts.append(t.cast(str, method.parent.name) + t.cast(str, method.normal_name)[11:])

After this PR, these are all eliminated, without sacrificing safety:

if optStart is not None:
    prefixes = [method.name]
    if method.name == "constructor":
        prefixes.append(method.parent.name)
    for i in range(optStart, len(method.arguments)):
        argText = ", ".join(arg.name for arg in list(method.arguments)[:i])
        for prefix in prefixes:
            texts.append(prefix + "(" + argText + ")")

texts.append(method.normal_name)
if method.name == "constructor":
    texts.append(method.parent.name + method.normal_name[11:])

tabatkins avatar Jun 23 '22 21:06 tabatkins