Proposal: Add a scope property to target inline imports

[Poogles]

Opening this to see if you would be receptive to a PR before I do the work.

I'd like to propose adding a property to allow rules to target inline imports. This property would have 3 possible values:

1. all, this would be the default and the current behaviour where rules are checked against the entire scope.
2. main, this would limit the checking of rules to imports within the main scope.
3. not main, this would limit the checking of rules to outside the main scope.

Some examples that would cover the above.

All

This is the current behaviour, if we had a rule setup that disallowed the importing of badpackage using the default all scope. Both of the below imports would trigger a rule violation.

import bannedpackage

def foo():
    import bannedpackage

Main

This blocks importing of a package into the main scope. Similarly if we had a rule setup that disallowed the importing of b adpackage but with the scope value set to main.

import bannedpackage  # this would trigger a rule violation.

def foo():
    import bannedpackage  # this would not as it's outside the main scope.

Not Main

This blocks importing of a package outside of the main scope. Similarly if we had a rule setup that disallowed the importing of badpackage but with the scope value set to not main.

import bannedpackage  # this is fine.

def foo():
    import bannedpackage  # this would trigger a violation.

I'm not set on the names so any suggestions are welcome. I can add examples as to why you would want to do this but I think they're fairly common (or at least not unusual) patterns where lazy loading of modules is required.

[seddonym]

Hi! Thanks for reaching out. I don't totally understand the proposal yet though. Would you be able to provide an example of how this might behave?

[Poogles]

Hi! Thanks for reaching out. I don't totally understand the proposal yet though. Would you be able to provide an example of how this might behave?

I've updated the opening post with some examples, I hope that explains things a bit better. I can extend them to include proposed config and more concrete examples, but I hope they get the point across for now.

[seddonym]

Ah right - I usually call these 'inline imports' or sometimes 'runtime imports'. Interesting idea!

One way of doing this would be a bit like the exclude_type_checking setting which is something you can specify in the top-level configuration rather than in individual contracts. There could be an inline_imports setting, say, that was either include|exclude|only. That would be easier to implement than having it as metadata on every import, say - and if you really needed different contracts to check different things, you could have two contract files and run import linter twice.

One thing to be aware of - I'm planning to reimplement the graph-building code (which is actually in the Grimp library, which I also maintain). At the moment it is written in Python and uses the ast package (from the standard library) to build an abstract syntax tree of all the code, before figuring out the imports. That is, I suspect, a large overhead for figuring out imports. So I'm planning to rewrite it in Rust, with a simpler algorithm that just finds import statements.

It's possible that I'll decide not to support features like this (and the exclude_type_checking one) in the Rust version. That might mean that Import Linter would fall back to a slower implementation if this configuration was set.

A good place to start looking at implementation is here.

[Poogles]

inline imports

Those are the words I was looking for.

One thing to be aware of - I'm planning to reimplement the graph-building code (which is actually in the Grimp library, which I also maintain). At the moment it is written in Python and uses the ast package (from the standard library) to build an abstract syntax tree of all the code, before figuring out the imports. That is, I suspect, a large overhead for figuring out imports. So I'm planning to rewrite it in Rust, with a simpler algorithm that just finds import statements.

Are you going to want to essentially maintain two code bases for this functionality?

A good place to start looking at implementation is here.

So I've had a quick look and it seems fairly simple to follow (awesome), my initial idea would be to add some kind of scope property to grimp.domain.valueobjects.DirectImport which allows differentiation of imports between the main and inline level. The ImportGraph chain methods would then need a follow_inline_imports boolean adding to allow tracking, plus a few changes to ForbiddenContract here. I'm probably missing some things here but that broadly looks to be the change set, it adds quite a bit of complexity for what is a somewhat niche requirement so I can understand if you wouldn't be too keen on accepting the changes. Any thoughts?

[seddonym]

Are you going to want to essentially maintain two code bases for this functionality?

I'd rather not 😄 . But it's a fairly isolated part of the code so it should be fairly straightforward to switch between them. I do get that some use cases require more sophisticated static analysis, but for me the priority is speed (I run Import Linter on a code base with several million lines of code, and it takes a couple of minutes to build the graph).

[seddonym]

add some kind of scope property to grimp.domain.valueobjects.DirectImport

I would much prefer the inline imports scanning mode to be limited to the ImportScanner - i.e. add an extra inline_imports_mode argument to the scan_for_imports method. This could take an enum with the different modes.

If we allow the concern of inline imports to leak onto the DirectImport object, it could complicate the rest of the code base as it has to check whether or not that metadata is available on the object - and, depending on the scanning mode that is used, it may not be. I think better to just build it internal to the ImportScanner (and its callers). For the time being at least I would prefer the mode to be an option that is passed when the graph is built, rather something that can be inspected on each import after building. Does that make sense?

[Poogles]

Does that make sense?

Yeah it does.

Given the focus on performance maybe let's park this today and come back and review again once the grimp to rimp migration is done as that might open up some other solutions?

[seddonym]

Sounds good!