Add 'importbin' statement

[anguslees]

Add importbin statement. Similar to importstr but the result is an array of numbers (all integers 0-255).

[anguslees]

I wasn't confident on this line - is this correct? Is there a better value to return?

[sbarzowski]

It's correct, but it would be better to return an array of numbers specifically. We can add another hardcoded type for this (alongside anyArrayType):

• Here you can add a new type id:

  go-jsonnet/linter/internal/types/placeholder.go

  Line 15 in 74b3a1b

  anyArrayType

• Here you actually define it:

  go-jsonnet/linter/internal/types/graph.go

  Line 131 in 74b3a1b

  g._placeholders[anyArrayType] = concreteTP(TypeDesc{

• Here's another place which could benefit from it:

  go-jsonnet/linter/internal/types/stdlib.go

  Line 9 in cb9061e

  arrayOfNumber := anyArrayType

[anguslees]

Thanks for the hints. Added numberArrayType and a testcase.

[coveralls]

Coverage decreased (-0.1%) to 67.694% when pulling 26c3f96 on anguslees:importbin into fb6c700 on google:master.

[sbarzowski]

Good stuff. We need C++ support before documenting this, but it's not a blocker for merging this here.

[sparkprime]

Strategically, I'm tempted to say we could merge this without C++ support and document that as an explicit limitation of the C++ implementation. What do you think?

[anguslees]

It suddenly occurred to me that this probably breaks the 'repeated MakeContents(foo) produces the exact same Contents' rule. Might need to change the Contents representation to store both a []byte pointer and a string pointer, or improve the 'is the same' check.

[anguslees]

False alarm? I think there's no problem here, and we're good to proceed.

---

More info: I think we get a different Contents even when called with the same string, ie:

foo := "foo"
a, b := MakeContents(foo), MakeContents(foo)
a != b

... because of the above line of code ... but afaics this is ok.

Afaics the intended semantics (enforced by importCache.importData()) are that Importer.Import returns a copy of the same Contents object for the same foundAt value -- not a new Contents object made from the same string (subtle difference).

ie: Importer.Import() is meant to do this:

// Good example
func (i *myimporter) ImportData(from, path string) (contents Contents, foundAt string, err error) {
        foundAt = i.resolvePath(from, path)
        if contents = i.cache[foundAt]; contents == nil {
                contents = MakeContents(i.readContents(foundAt))
                // Important! Return (a copy of) the same `Contents` for a given `foundAt`
                i.cache[foundAt] = contents
        }
        return
}

Not this:

// Wrong/bad example
func (i *myimporter) ImportData(from, path string) (contents Contents, foundAt string, err error) {
        foundAt = i.resolvePath(from, path)
        var stringData string
        if stringData, ok = i.dataCache[foundAt]; !ok {
                stringData = i.readContents(foundAt)
                i.dataCache[foundAt] = stringData
        }
        // Wrong! Now we have a 'new' Contents object for the same `foundAt`
        contents = MakeContents(stringData)
        return
}

[sbarzowski]

Yes, exactly. We enforce that it's the same Contents object.

(It encourages efficient caching by avoiding unnecessary copies and we can quickly compare pointers for equality instead of going over the whole string).

[sbarzowski]

Strategically, I'm tempted to say we could merge this without C++ support and document that as an explicit limitation of the C++ implementation. What do you think?

Here are the options I see:
(a) Merge this and then implement importbin on C++ side (which shouldn't be very hard).
(b) Deprecate the C++ implementation and don't spend much effort porting features there anymore (while enforcing that everything that works on C++ side, works in go-jsonnet as well). I wouldn't make an exception just for importbin. Either we want feature-compatibility or we drop it.

[anguslees]

C++ PR: google/jsonnet#976

I note there's a Rust PR already in flight too CertainLach/jrsonnet#77. The Rust implementation looks very nice. As a long-term support plan, I vote for dropping the C++ library in favour of the Rust-produced C bindings. We can discuss which project (go or Rust) produces the 'official' /usr/bin/jsonnet CLI tool.