Skip to content
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

Remove optionality from name on all reasonable constructs. #77

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tabatkins
Copy link
Contributor

@tabatkins tabatkins commented Jun 3, 2022

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
Copy link
Contributor Author

tabatkins commented Jun 23, 2022

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:])

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant