brownplt / pyret-lang

The Pyret language.
Other
1.07k stars 111 forks source link

Transitive and overly eager data includes are harmful #1643

Closed jpolitz closed 2 years ago

jpolitz commented 2 years ago

Consider this module:

provides-less-than-data.arr

provide:
  type D
end

data D:
  | a(field :: E)
end

data E:
  | other
end
include file("./provides-less-than-data.arr")

This include fails with

The identifier `a` at file:///Users/joe/src/pyret-lang/importer.arr:1:0-1:45 is not provided as a value in the import at file:///Users/joe/src/pyret-lang/importer.arr:1:0-1:45
The identifier `is-a` at file:///Users/joe/src/pyret-lang/importer.arr:1:0-1:45 is not provided as a value in the import at file:///Users/joe/src/pyret-lang/importer.arr:1:0-1:45
The identifier `other` at file:///Users/joe/src/pyret-lang/importer.arr:1:0-1:45 is not provided as a value in the import at file:///Users/joe/src/pyret-lang/importer.arr:1:0-1:45
The identifier `is-other` at file:///Users/joe/src/pyret-lang/importer.arr:1:0-1:45 is not provided as a value in the import at file:///Users/joe/src/pyret-lang/importer.arr:1:0-1:45
The identifier `E` at file:///Users/joe/src/pyret-lang/importer.arr:1:0-1:45 is not provided as a type in the import at file:///Users/joe/src/pyret-lang/importer.arr:1:0-1:45

The reason for this is that the module system lists all mentioned datatypes in the serialized module. This is important for the type checker to be able to see all the types (especially if they are used across module boundaries).

However, this means that an include-ing module sees all the datatypes and tries to import them all.

You can always get around this by introducing an intermediate module that cleans up and re-exports only what's needed. But the current behavior sucks because if you add a new helper datatype to an existing module and reference it in certain ways, all of a sudden it will either (a) need to be exported, causing potential shadowing errors or (b) cause errors on include.

I think it's objectively bad that include could ever fail when the module itself is well-formed. The right solution is some kind of flag on data definitions in serialized modules for whether they should be considered by include.

jpolitz commented 2 years ago

(This hurt live users, because the variant grouped ended up exported from chart.arr, which was a name used in the :DS teachpack. It was a trivial fix for us, but it underscores that this behavior isn't safe in general.)

blerner commented 2 years ago

We had decided this was the intended behavior of include because we couldn't figure out how to get more fine-grained control over exporting/importing datatypes to work, right? I agree it was a kludge, but do we have any better ideas how to finesse this?