mojo icon indicating copy to clipboard operation
mojo copied to clipboard

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

Open gabrieldemarmiesse opened this issue 9 months ago • 3 comments

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 avatar May 05 '24 13:05 gabrieldemarmiesse

does import something as _something work?

helehex avatar May 05 '24 13:05 helehex

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.

JoeLoser avatar May 05 '24 14:05 JoeLoser

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.

JoeLoser avatar May 12 '24 16:05 JoeLoser