Skip to content
This repository has been archived by the owner on Oct 9, 2019. It is now read-only.

Test is throwing exception: "TypeError: shrink1 is not a function" #227

Open
rhofour opened this issue Dec 31, 2017 · 8 comments
Open

Test is throwing exception: "TypeError: shrink1 is not a function" #227

rhofour opened this issue Dec 31, 2017 · 8 comments

Comments

@rhofour
Copy link

rhofour commented Dec 31, 2017

After expanding one of my fuzzers slightly my test began failing because of an exception.

This test failed because it threw an exception: "TypeError: shrink1 is not a function"

It appears shrink1 is an internal function in elm-test, but I don't know enough about how elm-test works under the hood to know why this might be happening.

Commit that causes crash: rhofour/elm-comidi@0d62c8f

@rtfeldman
Copy link
Member

rtfeldman commented Jan 1, 2018

I was able to reproduce this by checking out that commit. 👍

When I ran elm-test on that commit, the generated JS (elm-comidi/elm-stuff/generated-code/elm-community/elm-test/elmTestOutput.js) included this:

var _elm_community$shrink$Shrink$merge = F3(
	function (shrink1, shrink2, a) {
		return _elm_community$lazy_list$Lazy_List$unique(
			A2(
				_elm_community$lazy_list$Lazy_List_ops['+++'],
				shrink1(a),
				shrink2(a)));
	});

This is the only place anything named shrink1 got called as a function, and I hacked the elm-stuff source of elm-community/shrink to verify that this is indeed the call site that's breaking.

The offending merge function's shrink1 argument is a function, so that's to be expected.

When I hacked a Debug.log on shrink1 into the elm-community/shrink source, it printed out "<internal structure>" rather than "<function>" like I would have expected if it were a function.

No idea what the internal structure might be though, or how it managed to get there... 🤔

@rtfeldman rtfeldman added the bug label Jan 1, 2018
@rtfeldman
Copy link
Member

rtfeldman commented Jan 1, 2018

I thought maybe the problem was the Node test runner doing funky hacks, but I tried running this in the HTML runner and it's reproducible there too!

I stepped through with the browser dev tools, and the value of shrink1 at that point in execution is, somehow, undefined. (shrink2 is fine though; it's function _newlandsvalley$elm_comidi$MidiTest$shrinkMidiRecordingSameFormat())

Considering Elm doesn't even support a notion of an undefined value, something has definitely gone off the rails under the hood here...most likely in some Native code somewhere.

@rhofour
Copy link
Author

rhofour commented Jan 2, 2018

There shouldn't be any native code involved. elm-comidi and elm-test don't have any. If it fails in the HTML runner then that should rule out the Node runner.

Does that mean this is an Elm bug or is there some other alternative?

@rtfeldman
Copy link
Member

Definitely seems like an Elm bug to me!

@mgold
Copy link
Member

mgold commented Jan 3, 2018

Marking as "out of scope" to indicate upstream (language) issue.

@rtfeldman
Copy link
Member

Evan commented on Slack:

Known bugs that could potentially do that are (1) cyclic values that get generated in a bad order and (2) TCO that involves capturing variables in closures.
So if the bug involves recursively defined shrinkers (or decoders, generators, etc.) then that is probably the issue.
You can sometimes get it to “work” by reordering things in source.
I addressed this in my latest pass over the 0.19 compiler code.

@rtfeldman
Copy link
Member

Turns out there are at least two compiler bugs here.

@eeue56 commented on Slack:

elm-test shouldn't be using the elm-community/shrink solution any more -- the problem might fix itself if it's switched to eeue56/elm-shrink since that's based on a different lazy-list implementaiton, though I doubt it

On that branch the tests request elm-community/shrink in elm-package.json but use the latest elm-community/elm-test, which depends on eeue56/elm-shrink instead of elm-community/elm-shrink.

The generated exact-dependencies.json shows that it's getting both eeue56/elm-shrink and elm-community/shrink (this is on a local version where I'm using the HTML test runner):

{
    "elm-community/lazy-list": "1.0.0",
    "elm-community/elm-test": "4.2.0",
    "mdgriffith/style-elements": "3.4.1",
    "elm-community/shrink": "2.0.0",
    "elm-lang/virtual-dom": "2.0.4",
    "eeue56/elm-lazy": "1.0.0",
    "mgold/elm-random-pcg": "5.0.2",
    "elm-lang/lazy": "2.0.0",
    "eeue56/elm-lazy-list": "1.0.0",
    "elm-lang/dom": "1.1.1",
    "elm-lang/html": "2.0.0",
    "eeue56/elm-shrink": "1.0.0",
    "elm-community/html-test-runner": "1.0.6",
    "Bogdanp/elm-combine": "3.1.1",
    "elm-lang/window": "1.0.1",
    "Skinney/murmur3": "2.0.6",
    "elm-lang/core": "5.1.1"
}

And yet, the code is still clearly calling _elm_community$shrink$Shrink$merge - which is causing the crash.

This is because MidiTest.elm itself does import Shrink exposing (Shrinker) and builds custom shrinkers which it uses in fuzzers. Since there is only one package in its elm-package.json that exposes a Shrink module—elm-community/shrink—it uses that.

Here's where the other bug comes in. These custom shrinkers are being built using elm-community/elm-test and then successfully passed to Fuzz.custom, even though Fuzz.custom expects a Shrinker from eeue56/elm-shrink rather than the Shrinker from elm-community/elm-shrink it's receiving instead. Those Shrinker types have the same name, but they come from different packages and should not be compatible. MidiTest.elm should not compile.

Indeed, if you remove elm-community/elm-shrink from elm-package.json, then MidiTest.elm no longer compiles because there are no packages in elm-package.json that expose a Shrink module. Likewise, if you add both elm-community/shrink and eeue56/elm-shrink to elm-package.json, then the compiler complains that it can't import Shrink because it found multiple modules named Shrink. Both of those make sense.

The bug is specifically that the compiler is okay with import Shrink (which is totally fine on its own; it's not an ambiguous import when only elm-community/elm-shrink is in elm-package.json), but then later it doesn't realize that the Shrinker type from this particular Shrink module should be incompatible with the Shrinker type that Fuzz.custom is requesting, because they come from different packages.

Replacing elm-community/shrink with eeue56/elm-shrink in elm-package.json resolves the discrepancy...but, as @eeue56 predicted, the crash persists - the only difference is that it's now (correctly) coming from _eeue56$elm_shrink$Shrink$merge instead of _elm_community$shrink$Shrink$merge.

@rtfeldman
Copy link
Member

Okay, apparently the 0.19 compiler already includes package name as part of type unification, so neither bug should happen anymore.

I'm gonna leave this open as a "let's circle back to it and make sure it's no longer reproducible post-0.19," but I don't think any changes are needed on the elm-community/elm-test side. 😄

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants