Bug? Inline function locals/params shadow namespace members - Palette.text resolves to local text

[ustk]

@dannytaurus Nice one

[ustk]

When the time comes for @Christoph-Hart to merge/manage the current pile of PRs, we'll not see him until his beard touches the ground... Let's hope he has a strong pilosity! (and a good stock of "Hopfenkaltschale" )

[dannytaurus]

@ustk Yeah, definitely be good to get the nod from Christoph on a lot of these PRs.

Claude does a great job of making and testing bug fixes, but only Christoph really knows where the dragons are hiding in the code!

[dannytaurus]

@Christoph-Hart Thanks for the proper fix. (Here in case anyone missed it)

Claude says there's still one case that your fix doesn't catch:

Calling Palette.text where Palette has no text but a local text is in scope will silently bind to the local instead of erroring. Degenerate case (the code is already wrong at that point), but just thought you should know. Maybe you saw it and elected not to cover it.

[David Healey]

Been trying to figure out for the last few hours why one of my projects keeps segfaulting when opening it. Tracked it down to this commit. I'm still digging into it but this is what Claude "thinks"

After the refactor, parseQualifiedNamespaceExpression(ns) only checks three specific things — inline function, const object, registered var — and if member doesn't match any of those:

return parseFactor(nullptr); // <-- discards ns entirely!

That's the bug: any Namespace.member reference where member isn't literally one of those three exact kinds is now silently reparsed as if it were a completely unrelated global identifier, instead of correctly resolving inside the namespace. Depending on what else in the script happens to share that name globally, this can make an expression quietly resolve to the wrong object rather than erroring

[Christoph Hart]

@David-Healey hmm a namespace can contain only const regs or inline functions…

[David Healey]

@Christoph-Hart What about regular functions? (paint routines, broadcasters, and callbacks I'm thinking)

[David Healey]

@Christoph-Hart Changing return parseFactor(nullptr); to return parseFactor(ns); solves the crash but I don't know if that negates the purpose of the validator.

[Christoph Hart]

@David-Healey normal functions are var declarations in disguise and previously leaked to the global namespace without warning - that‘s one of the reasons why I added the yellow warning stuff

[David Healey]

@Christoph-Hart Does that mean no paint routines, mouse callbacks, etc. are allowed in namespaces?

[David Healey]

I think I've narrowed it down.

I have a namespace called CoreLookAndFeel that is included with every project. And it contains "overrides" like this

		pnlBody.setPaintRoutine(function(g)
		{
			if (isDefined(LookAndFeel.drawBody))
				return LookAndFeel.drawBody();
	
			var a = this.getLocalBounds(0);

			g.fillAll(this.get("bgColour"));
	
			if (!isDefined(LookAndFeel.style.useNoise) || LookAndFeel.style.useNoise)
				g.addNoise({alpha: 0.025, scaleFactor: 2.0, area: obj.area, monochromatic: true});
		});

In the project that's crashing I do have a LookAndFeel namespace, but it doesn't contain a drawBody function or a style object. I think that's the trigger for the crash I'm seeing.

[Christoph Hart]

@David-Healey ah so basically Namespace.nonexistingProperty crashes?

[David Healey]

@Christoph-Hart That's what I thought, but it's inconsistent. I'll spend some more time poking it and see if I can get a reproducible example.

[David Healey]

@Christoph-Hart Just doing a little test and it seems that inline functions are still leaking?

[David Healey]

@Christoph-Hart Yep that's the issue. The isDefined returns the wrong result so the script then tries to call a function that doesn't exist.

namespace FirstNameSpace
{
}

namespace SecondNameSpace
{
	inline function myFunc()
	{
		if (isDefined(FirstNameSpace.myFunc))
			FirstNameSpace.myFunc();
	}	
}

SecondNameSpace.myFunc();

[Christoph Hart]

@David-Healey ah yes that's a bit more complicated - the resolver for the first namespace doesn't work so it basically ignores the namespace qualifier and resolves the function call to itself so it creates a recursive loop.

In order to reproduce the faulty behavior without the crash you can do this:

namespace FirstNameSpace
{
	const var myFunc2 = 90;

}

namespace SecondNameSpace
{
	inline function myFunc()
	{
		if (isDefined(FirstNameSpace.myFunc))
		{
			Console.print("exists");
		}
		else
		{
			Console.print("doesn't exist");
		}
			
	}	
}

SecondNameSpace.myFunc();

The suggested fix of passing in the current namespace is not a real fix, it might solve your immediate problem, but the correct option would be to return an undefined expression like this:

match(TokenTypes::identifier);
return parseSuffixes(new Expression(location));

then this code would work as expected (returning "doesn't exist"). However this might silently break existing code that used this construct to falsely refer to non-namespaces data through a namespace qualifier so I'm a bit hesitant to push this.