Imports "leak" across to other imported files

[dgelessus]

Using kaitai-struct-compiler 0.9-SNAPSHOT501381150.

It seems that specs imported by one spec are also implicitly imported for all other specs. This can lead to an imported spec compiling even though it doesn't import all needed types (because the only places that use it happen to also import the necessary types). Short example:

one.ksy:

meta:
  id: one
seq:
  - id: something
    size: 4

two.ksy:

meta:
  id: two
  # Note: missing import of `one`
seq:
  - id: one_thing
    type: one # Note: `one` is not imported in this spec, so this should cause a compile error

toplevel.ksy:

meta:
  id: toplevel
  imports:
    - one
    - two
seq:
  - id: another_one
    type: one
  - id: another_two
    type: two

one.ksy is valid. two.ksy is invalid (because it uses one without importing it) and does not compile. toplevel.ksy itself is valid, but it imports the invalid two.ksy. Despite this, compiling toplevel.ksy does not produce an error, because toplevel's import of one leaks into two. (If you remove the usage of one from toplevel.ksy, it will fail to compile, because then two cannot find the required one type anymore.)

Real example: kaitai-io/kaitai_struct_formats#126 (review)

[GreyCat]

Hmm, I'm not really sure how to deal with this one. On the one hand, you've clearly demonstrated the problem. On the other hand, this is how class importing is structured now, and from top of my head, I can't think of a better way to do it.

Currently, it loads everything into one large collection of types (ClassSpecs), and every type loaded can be used by every type. So, in a way, this is by design. OTOH, we can probably enforce extra visibility checks, i.e. class will be loaded, but not accessible from a type unless that type has relevant imports directive.

[webbnh]

I would be inclined to start by "reading the rules".... Is there a requirement that each imported file itself be valid? Or, is it only required to be valid in the context of the result of the imports? (I can understand the desire for them each to be valid, but does it really matter?)

Also, if a KSY file includes a KSY file which includes a KSY file, does/should the top level file end up with the transitive closure of all the includes? Or, are the includes scoped in some way? Can a KSY file include another KSY file more than once (directly)? What about indirectly?

And, the real nut: what if my top level file includes two second level files each of which include different versions of the same third level file? What if the two third level files are compatible with each other? What if they conflict? If they are compatible, we can just let one occlude the other; however, if they conflict, then we need to have scoped inclusions in order for it to work (that is, each of the second level files is valid KSY, so shouldn't they each be able to be included into another KSY file??).

How much headache are we interested in signing up for, here. I'm satisfied with the idea that all the definitions included by all the included files go into one pool (in which they must all be mutually compatible) from which any of them can draw.... (But, then, my brain is already full of other things, and this is more than I can actually worry about....)

[KOLANICH]

Is there a requirement that each imported file itself be valid?

Each ksy is compiled into a separate module of target language. So, yes.

Also, if a KSY file includes a KSY file which includes a KSY file, does/should the top level file end up with the transitive closure of all the includes?

Depends on target language. In C++ if a file a includes a header b including a header c, c is included in a in the end.

But in the languages having modules and imports it depends a lot.

And, the real nut: what if my top level file includes two second level files each of which include different versions of the same third level file.

Should be forbidden. There is always a single version - the latest one. All other versions are obsolete and unsupported. If something is incompatible to the latest version of its dependency - it is a bug in that software, not in the latest version of the dependency. That bug must be fixed.

[dgelessus]

I think we might have different expectations of what an "import" means...

Coming from languages like Python and Java, when I import something in my file, I expect it to appear only in my file. I don't want my imports to appear in files that import my file, or in other files that I import, or in other files that happen to be imported together with my file. Basically, I expect each file to see only what it defines itself and what it directly imports.

In some languages this is handled differently. For example the C-family languages are the exact opposite - imports there are done using a purely text-based preprocessor #include, which means that any definitions you include are also visible to anything included afterwards (by you or someone else).

However I'm not sure how it would be useful to emulate the C-style behavior in Kaitai. Is there any legitimate reason why you want to use a type in a Kaitai spec, but not import it yourself, and instead have another file import it?

And, the real nut: what if my top level file includes two second level files each of which include different versions of the same third level file?

I'm not sure what situation you're describing here exactly, Kaitai doesn't have a concept of "versions" of a spec... If you're talking about two files with the same name but in different directories, that's already a problem with the current system, right? I haven't tested it, but I'd expect Kaitai to give me an error, since there would be a name conflict.

This could of course be change if imports were made to not leak. Then, for example, if you have a spec a that imports dir1/x and b that imports dir2/x, you would ideally be able to import a and b together without getting a conflict from the two xes. I don't think I would need this in practice though.

[webbnh]

Is there a requirement that each imported file itself be valid?

Each ksy is compiled into a separate module of target language. So, yes.

But, today, that compilation is accomplished despite the fact that the imported file is not itself valid (because of the transitive closure)....

There is always a single version - the latest one.

But, these two "versions" could be entirely independent specs which happen to use the same names for one or more of their types or other definitions. So, it's not a matter of getting the latest one -- they could both be the "latest" one and still conflict. Contrariwise, it's possible that they are in fact different versions of the same spec, but the latest one removed or renamed a definition which one of the two including specs depends on -- both uses of both versions could still be valid, but the two definitions still conflict.

This could of course be change if imports were made to not leak. Then, for example, if you have a spec a that imports dir1/x and b that imports dir2/x, you would ideally be able to import a and b together without getting a conflict from the two xes.

This is what I was getting at. If there's no leak, then it doesn't matter whether the imports of two subordinates might conflict with each other. However, if the top level receives the full transitive closure, then all subordinate's imports must be compatible, even though there is no reason to expect the developers of those specs to have the required foreknowledge to have ensured this.

This is not a new problem. And, as we've pointed out here, different languages have chosen to address it in different ways. And, it's possible that KS can/has addressed this as well -- I think that there are ways to use opaque types which are defined by other KS specs to avoid these conflicts, but it's slightly tricky and I haven't thought it all the way through.

I suspect that the summary here is that KS is following the C Language model, where the "compilation unit" contains all the definitions, and, where that's a problem, the developer has to place the definitions in separate compilation units which are properly encapsulated and later combined at link-time.

[KOLANICH]

But, these two "versions" could be entirely independent specs which happen to use the same names for one or more of their types or other definitions.

This should also be forbidden. We need namespaces, but currently they are not implemented.

Types which are not in separate files don't clash to each other. But 2 files with the same id in meta would clash.

[Mingun]

Hmm, I'm not really sure how to deal with this one. On the one hand, you've clearly demonstrated the problem. On the other hand, this is how class importing is structured now, and from top of my head, I can't think of a better way to do it.

Currently, it loads everything into one large collection of types (ClassSpecs), and every type loaded can be used by every type. So, in a way, this is by design. OTOH, we can probably enforce extra visibility checks, i.e. class will be loaded, but not accessible from a type unless that type has relevant imports directive.

To fix that issue you need to track not only class definition, but also a list of sources -- in which KSYs it was imported directly. When resolve reference from the type T from file F.ksy, we need to check that the returned ClassSpec was actually imported in this file and not leaked from other file:

/** Resolves reference to a type that is used in expression inside `inType` */
def resolveType(typeRef: String, inType: ClassSpec): ClassSpec = {
  val resolved = classSpecs.get(typeRef) // returns ClassSpec
  if (resolved.sources.contains(inType.topLevelType.id)) {
    return resolved
  }
  throw new UnknownType(typeRef)
}