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

[BUG] Functions, traits, structs and aliases are leaking into builtins and are importable from anywhere #2529

Open
gabrieldemarmiesse opened this issue May 5, 2024 · 3 comments
Labels
bug Something isn't working mojo-repo Tag all issues with this label

Comments

@gabrieldemarmiesse
Copy link
Contributor

gabrieldemarmiesse commented May 5, 2024

Bug description

As the title says, importing anything without a leading underscore in "./stdlib/builtin/anything.mojo" insert it into the list of stuff that does not need to be imported globally.

The fix should be to make anything that's defined in https://github.com/modularml/mojo/blob/nightly/stdlib/src/builtin/_init_.mojo available globally, but not more. And in this file, we will import all the things that should be auto-imported.

Steps to reproduce

Consider this code:

def main():
    print(now()) # I want this to call time.now()

you might think that it will not work. Well, if someone, by mistake, imports now() into any of the builtin files, this will work.
e.g. Add from time import now in stdlib/src/builtin/hash.mojo, then run

./stdlib/scripts/build-stdlib.sh && MODULAR_MOJO_NIGHTLY_IMPORT_PATH=./build mojo trying_stuff.mojo

It will happily compile the code and print

13050285013500

Imports are all messed up in the stdlib because of this. When doing refactorings, suddently from functions are not auto-imported anymore and sometime some functions are auto-imported while we don't want them to be.

We should auto-import everything defined in a single file, not a whole package. After all, there are not that many things that should be auto-imported in mojo.

Suggestion of the fix process

Since changing the imports in the whole stdlib and the internal codebase might cause a huge diff and a lot of conflicts, I will suggest the following:

  • we populate stdlib/src/builtin/__init__.mojo with the current list of auto-imported names (so too much names, so we don't have to fix everything at once)
  • The compiler devs change the rule for auto-imported names. Auto-import everything in stdlib/src/builtin/__init__.mojo instead of everything in stdlib/src/builtin/*.mojo. Since we filled stdlib/src/builtin/__init__.mojo with everything auto-imported before, no other changes are necessary.
  • We remove progressively names from stdlib/src/builtin/__init__.mojo that shouldn't be auto-imported. We can even do one name by one name to avoid too many conflicts or too many changes in one PR.

System information

ubuntu 22.04 in wsl2
modular 0.7.2 (d0adc668)
mojo 2024.5.323 (1d9276ea)
@gabrieldemarmiesse gabrieldemarmiesse added bug Something isn't working mojo-repo Tag all issues with this label labels May 5, 2024
@helehex
Copy link
Contributor

helehex commented May 5, 2024

does import something as _something work?

@JoeLoser
Copy link
Collaborator

JoeLoser commented May 5, 2024

does import something as _something work?

Yes, this is how you override the behavior of builtin symbols. We do this internally since you can't form overload sets across files for common identifiers, such as max and min in addition to have tighter control saying "use my function" not that one from the builtin with the same identifier.

I'm not a huge fan of the whole transitive closure set for builtin imports. I don't know why it was chosen this way originally. I'll bring it up in the weekly Mojo deep dive meeting this coming week.

@ematejska ematejska added mojo-repo Tag all issues with this label and removed mojo-repo Tag all issues with this label labels May 6, 2024
@ematejska ematejska added the mojo-stdlib Tag for issues related to standard library label May 7, 2024 — with Linear
@JoeLoser JoeLoser added mojo-lang Tag for all issues related to language. and removed mojo-stdlib Tag for issues related to standard library labels May 12, 2024
@JoeLoser
Copy link
Collaborator

Reassigning to Lang as this requires compiler changes in how we import builtins. There's not much the library itself can do short of providing a smaller transitive closure set of builtins/prelude.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working mojo-repo Tag all issues with this label
Projects
None yet
Development

No branches or pull requests

4 participants