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

Allow builtin atoms to be shadowed by user type definitions #908

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

CAD97
Copy link
Contributor

@CAD97 CAD97 commented Aug 3, 2021

WIP, but potentially working already; needs judicious tests. Addresses part of #682.

Based on the sketch in #682, the to-do list is roughly

  • Centralize atom spotting into Types
    • Remaining: parse_repr_attribute, parse_int_suffix, implicit_impl, and centralized spotting; all seem legit
  • Only spot atoms that don't already have a user-defined resolution
  • Test a bunch of cases to ensure that it works

If anyone would like to pick this up, feel free to do so (and/or PR tests onto my branch)!

Note that this does not handle builtins (that is, Box, UniquePtr, SharedPtr, WeakPtr, Vec, CxxVector, and str), which are different from atoms (that is, bool, c_char, uNN, iNN, fNN, CxxString, String). Those will take more thorough refactoring to weaken similarly.

@adetaylor
Copy link
Collaborator

Thanks for working on this!

@CAD97
Copy link
Contributor Author

CAD97 commented Aug 4, 2021

The difference between an atom and a builtin is that an atom is a non-generic type, and a builtin is a generic type[1]. As such, builtins are parsed specially, and are unique kinds of Types.

As such, my proposal for how to handle builtin shadowing is a bit interesting: don't! Or rather,

  • Don't prohibit use of builtin names in check_reserved_name anymore.
  • After type collection, spot every use of builtin names as idents, along with spotting atoms.
  • If a builtin name is used as an ident but not defined, instead of an "undefined type" error, emit a helpful "did you mean the builtin" error.
  • If a builtin name is used as an ident and defined, scan for any uses of the actual generic builtin type, and if found, emit an error indicating the name collision.

The key insight here is that cxx doesn't actually mind if we define types with the reserved builtin names (as far as I understand it); the issue comes for the user juggling that UniquePtr<T> and UniquePtr are disparate types. As such, we can "just" allow both types to be defined, and then emit an error that this is weird, rather than building a more sophisticated system supporting more complicated cases.

[1]: str isn't. I say just leave str as a reserved name. Unfortunately bailing out here... means that CStr is also put in a similar spot. I think the full solution might be to not use Type::Str, but instead Type::Ref to an atom str, and the ref-atom is handled specially for str/CStr atoms.

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.

None yet

2 participants