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

[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.

[David Healey]

@Christoph-Hart I've merged the new commit and it no longer crashes, just shows some ugly errors. Is this intended as a stop-gap solution? Because isDefined() still returns true for the none-existent function.

Edit: Oh I see, it just stops the execution at the "error"

Tried your suggestion and appears to be working here, maybe a preprocessor definition (default true) to protect older buggy projects?