dyno: handle shadow scopes for 'private use'

[mppf]

This PR updates the dyno scope resolver to follow the current language design rules about shadow scopes. These evolved a bit after the dyno scope resolver was created (see PR #19306). See also this spec section.

To do so:

• added a field to VisibilitySymbols to indicate which shadow scope (if any) the use/imported symbols go into
• renamed doLookupInImports to doLookupInImportsAndUses and have it only consider symbols at the current shadow scope level
• created doLookupInAutoModules (pulled out of doLookupInImports)
• adjusted doLookupInScope to check the non-shadow scope and then the two shadow scopes
• adjusted doResolveUseStmt and doResolveImportStmt to provide a shadow scope value when constructing a VisibilitySymbols
• improved stringify/dump for VisibilitySymbols and ResolvedVisibilityScope
• added testing for some of these cases to testScopeResolve.cpp

This fixes a number of tests that fail with --dyno (which activates the dyno scope resolver):

modules/shadowing/issue-19167-3-x
modules/shadowing/issue-19167-5-x
modules/shadowing/var-shadows-private-import
modules/shadowing/var-shadows-public-import
modules/shadowing/var-shadows-public-use
visibility/import/rename/same_name_in_mod
visibility/import/rename/same_name_in_mod2

Reviewed by @DanilaFe - thanks!

• full local testing
• full local --dyno testing

[DanilaFe]

Looks good!

[DanilaFe]

Why int8_t instead of ShadowScope?

[mppf]

It saves 4 bytes of memory in each instance here due to the way that C / C++ have enums be int and the way that fields are aligned. If it is a 1 byte type, it can be stored next to the bool, instead of on the next 4-byte aligned word.

We could apply this space optimization in many other places in the uAST but somehow it seemed more important to me in this case, where the enum only stores 3 values. Thanks to our efforts at encapsulation, this field is private, so the type of it is not particularly important to most of the code.

[DanilaFe]

Might these three checks be somewhat elegantly combined into a for-loop for int8_t and VisibilitySymbols::ShadowScope? something like:

for (int8_t i = VisibilitySymbols::REGULAR_SCOPE; i < VisibilitySymbols::SHADOW_SCOPE_TWO; i++) {
  bool got = false;
  if (i == VisibilitySymbols::REGULAR_SCOPE) { /* lookupInScope logic */ }
  if (i == VisibilitySymbols::SHADOW_SCOPE_ONE) { /* auto-use logic */ }
  got |= doLookupInImportsAndUses(context, scope, resolving, r, name,
                                 onlyInnermost, skipPrivateVisibilities,
                                 VisibilitySymbols::SHADOW_SCOPE_TWO,
                                 checkedScopes, result);
  if (onlyInnermost && got) return true;
}

I'm on the fence about this suggestion, but the reason I like it is that it emphasizes the fact that we are searching each of the three shadow scopes in turn, though some have additional things to search.

[mppf]

Yes, I agree that it could be written that way. However, I feel that the comments adequately emphasize that we are searching each in turn (and if you have a suggestion for improving those comments, I'm happy to do so). At the same time, having a for loop that has conditionals to create different behavior for every index visited seems a bit of an anti-pattern as well.

So, let's leave it as-is.

[DanilaFe]

Would more descriptive names be better here, like CONTENTS_SHADOW_SCOPE and NAME_SHADOW_SCOPE? It's not like the scopes are numbered in the spec.

[mppf]

I know it might seem strange that it doesn't just say something like "Private Use Name Shadow Scope" and "Private Use Contents Shadow Scope". But there is a reason that I used these names. (It is debatable, but I have come to like the names I chose in the PR).

Anyway, here is the rationale.

This enum is within VisibilitySymbols. That type is a sort of normalized form for use and import statements. In fact, it does not directly indicate if it is referring to a use or import at all. Instead, it describes the behaviors that are requested. In particular, the Kind enum indicates if it's bringing in all contents or just the symbol etc. Related to that, if we had CONTENTS_SHADOW_SCOPE, that would be redundant with the Kind setting ALL_CONTENTS and it bakes the behavior of private use into the representation of the normalized form.

Instead, it makes more sense to me that the shadow scope names be independent of how private use behaves. The code that creates a VisibilitySymbols for a use or import is responsible for encoding the shadow scope behavior of use and import into this type. I think this approach has better separation of concerns.

However, I understand that a reader of this code might not understand why we have these shadow scopes at all. That is why I put in comments talking about private use. In a way, this is similar to the comments for the Kind elements. (Comparing Kind and ShadowScope, I am noticing that the new enum elements have comments that are not doxygen-ready. I will fix that presently.)