Application won't compile - "cannot read property 'n' of undefined" since 3.16.1

[antony]

Describe the bug

Upgrading an existing app to Svelte 3.16.1 causes client compilation to fail with the error Cannot read property 'n' of undefined.

I'm the second person to see this in the discord, it seems. I don't know where the error comes from or what the causes is (but I do see that the variable n is used a lot in recent Svelte commits - bb5cf9a).

Logs

ant@xeno  ~/Projects/beyonk-dashboard   master ●  npm run dev

[email protected] dev /home/ant/Projects/beyonk-dashboard
PORT=1233 NODE_CONFIG_ENV=${NODE_ENV} sapper dev

✗ client
Cannot read property 'n' of undefined

To Reproduce
I'm afraid I simply don't know, at present.

Expected behavior
The client should compile as normal.

Information about your Svelte project:

• Your operating system: Ubuntu 19.04

• Svelte version 3.16.1

• Rollup

Severity
Blocker. It's broken, and my app won't start.

Additional context
The app was previously using 3.15, and trying 3.16.0 wouldn't start either, due to an issue with reduce with no initial value.

[Conduitry]

Presumably #4063 was an incomplete fix. Please do update with a repro if you find one.

[ghost]

This script should try to compile all components in the project and allow the full stack trace and breakpoints using the built-in Node debugger.

Not sure if it needs tweaking for sapper though.

const svelte = require('svelte/compiler');
const fs     = require('fs');
const glob   = require('glob');

function do_compile(err,data) {
    if (err) {
        console.log("Couldn't read file:", err);
        process.exit(-1);
    }

    let result = svelte.compile(data);
}

function call_svelte(err, files) {
    if (err) {
        console.log("Glob error:", err);
        process.exit(-1);
    }

    files.forEach(f => {
        fs.readFile(f, {encoding:"utf8"}, do_compile);
    });
}

glob('src/*.svelte', call_svelte);
glob('src/**/*.svelte', call_svelte);

[mrkishi]

I believe this line is the issue:

svelte/src/compiler/compile/render_dom/Renderer.ts

Line 256 in 8a6abc9

({ operator, left, right } = x`${dirty} & /*${names.join(', ')}*/ ${bitmask[0].n}` as BinaryExpression);

We could temporarily fix it by checking for bitmask[0], but I don't know what causes this to happen in the first place... I believe the issue is that context_overflow is set after the Block and FragmentWrapper are created and they use dirty, but don't they also append to the context? So idk the right approach here.

[ghost]

@mrkishi,

This line should probably just initialize bitmask to contain an object since we'll always need at least one of them.

svelte/src/compiler/compile/render_dom/Renderer.ts

Line 209 in 8a6abc9

const bitmask: BitMasks = [];

Should be:

const bitmask: BitMasks = [{ n: 0, names: [] }]; 

Burning question is still how did the bitmask object get initialized not containing an index 0 in the first place!

[mrkishi]

Contrary to what I assumed earlier, I see that renderer.add_to_context() is called in a few places after initialization, during render. That'd explain why context_overflow is false even though there are more than 31 variables in the context.

[dimfeld]

Here's a reproduction: https://svelte.dev/repl/5357a38fcfc6441ebcf162fa5d307a12?version=3.16.1

This just imports the svelte-select library. I haven't reduced this to a minimal test case, but hopefully it helps you figure this out. It appears to be failing on the file List.svelte inside svelte-select.

<script>
	import Select from 'svelte-select';
</script>

[antony]

I took the code from @dimfeld 's repro (thanks!) and took the List.svelte component out and started to trim it back.

This is as minimal as I can get it without making the error disappear:

https://svelte.dev/repl/909f45b7a2234ad2804d3dcecd59a54b?version=3.16.1

It seems to occur when you have exactly 31 variables declared and a component declaration.

Note:

• The component declaration is essential
• The number of variables is essential (31)
• The component must be declared with opening and closing tags on different lines

[solidsnail]

• In continuation to what @antony said above, the issue is also caused by a carriage return of an empty component, like so:

<A>

</A>

The error goes away if you write <A></A>

[antony]

• In continuation to what @antony said above, the issue is also caused by a carriage return of an empty component, like so:

<A>

</A>

The error goes away if you write <A></A>

This is what I meant by point #3 above. It's not the sole cause though. You also need exactly 31 variable declarations.

[Conduitry]

Great, thanks for the minimal repro, folks!

edit: It looks like the necessary condition for this to manifest is actually that there be some content passed to the component. The carriage return isn't important, but it does count as content to be passed to the child component.

[Conduitry]

One way to address this looks like it would be to replace

if (!bitmask[i]) bitmask[i] = { n: 0, names: [] };

with

if (bitmask.length <= i) {
	for (let j = bitmask.length; j <= i; j++) {
		bitmask[j] = { n: 0, names: [] };
	}
}

so that we fill in the missing elements in the bitmask array, but I'm not sure whether this is just covering up a bug elsewhere.

[Conduitry]

Just was chatting with @mrkishi and it looks like the issue is that context_overflow is getting set too early, before everything has been added to the context. Here is where I think we are calling this.add_to_context() after we've already set context_overflow.

The obvious solution is to defer calculating context_overflow, or to make it a method and call that instead. I don't think that'll be masking a bug, as in my tests the new does_context_overflow() method wasn't getting called at all until after that call to add_to_context(). This does, however, reveal a bug in code-red related to the /* comments */ in the rendered that would have to be addressed first if we went that route.

Edit: I think it'll be safe just to move this.context_overflow = this.context.length > 31; after this.fragment.render(this.block, null, x#nodes as Identifier);. That's after the calls that add '$$scope' to the context and before the places where we're checking the value of context_overflow. There's still the code-red issue to address.

[Rich-Harris]

Fully admit that this part of the code is completely bewildering. The idea behind setting context_overflow before fragment.render(...) is that rendering will add a bunch of stuff to context that couldn't be part of a changeset — e.g. if you have exactly 31 variables, there's no overflow, but if you have this inside the component...

{#each things as thing}
  <p>{thing}</p>
{/each}

...then thing will get added to context, making its length 32. But because dirty is only tracking top-level changes, it doesn't affect whether or not there's an overflow.

Certainly possible — likely, even — that there's a much neater way to do all this without causing unnecessary overflows.

[Conduitry]

I also just noticed that the code-red issue I was seeing would be fixed in #4078, so I'm glad I don't have to worry about that. If I rebase moving this.context_overflow's assignment onto that branch, I don't have any problems.

But I also see now why that was an issue at all - we were now being overly cautious in saying there was a context overflow in many situations. Blargh. I dunno. I guess the issue is that some of the things added during render really do need to be part of the context and others do not.

[Rich-Harris]

Hmm. Well I'll merge #4078 as-is and cut a release, because I need it for work. We could certainly move the assignment down to guarantee correctness, and then worry about eliminating false positives later

[Conduitry]

Sounds reasonable. I'll rebase and re-open #4084 after you merge #4078.

[ghost]

I'd also initialize bitmask with the first index populated since we'll always need it anyway...

const bitmask: BitMasks = [{ n: 0, names: [] }];