Language pollution due to inherit()

[allejo]

By reading through inherit(), it seems to me that its purpose is to clone an object and copy over all its values.

The existing behavior right now is that Mercury is making a "copy" of hljs.QUOTE_STRING_MODE by using inherit. But because inherit() is just making a shallow copy, when STRING.contains.push(STRING_FMT); is called, Mercury is actually pushing STRING_FMT into hljs.QUOTE_STRING_MODE.contains.

highlight.js/src/languages/mercury.js

Lines 45 to 52 in 980ac21

	var ATOM = hljs.inherit(hljs.APOS_STRING_MODE, {relevance: 0});
	var STRING = hljs.inherit(hljs.QUOTE_STRING_MODE, {relevance: 0});
	var STRING_FMT = {
	className: 'subst',
	begin: '\\\\[abfnrtv]\\|\\\\x[0-9a-fA-F]*\\\\\\|%[-+# *.0-9]*[dioxXucsfeEgGp]',
	relevance: 0
	};
	STRING.contains.push(STRING_FMT);

At the moment, mercury is polluting armasm.

highlight.js/src/languages/armasm.js

Lines 63 to 65 in 980ac21

	hljs.C_BLOCK_COMMENT_MODE,
	hljs.QUOTE_STRING_MODE,
	{

[joshgoebel]

Could you provide a test case (in the test suite) to show the issue we're fixing here? Did you do any performance testing to see if this is a regression? inherit is used a lot, at run-time in addition to compile time.

Though I wonder if the "proper" fix for this might actually be for us to document that inherit PURPOSELY doesn't deep clone and in a case like this you'd need to deep clone contains manually. In that case though we might rename inherit so it's understood that it's shallow.

[joshgoebel]

We definitely need to freeze all the constants we're exposing from hljs though to prevent this clobbering.

[allejo]

+1 for freezing constants from hljs.

But wouldn't freezing constants just cause inherit to throw an error in this case? Is inherit intended to be shallow? If so, should mercury and languages be tasked with making their own deep copies?

[joshgoebel]

But wouldn't freezing constants just cause inherit to throw an error in this case?

Yes, because right now there is a HIDDEN bug... preventing the constants from being accidentally modified is a good idea regardless of whether inherit should be shallow or not.

Did you also look into why your tests are failing? It's possible grammars and the codebase in general expect the current shallow behavior.

Is inherit intended to be shallow?

Not sure, but it always has been. And I think I've seen grammars that assume so and work around it properly.

[allejo]

Did you also look into why your tests are failing? It's possible grammars and the codebase in general expect the current shallow behavior.

Looks like I was breaking RegExp objects inside of inherit causing those tests to break. However, even with just copying arrays looks like tests fail for clojure/json.

Should this be moved to an issue instead? I thought this was going to be a much simpler fix.

[joshgoebel]

Should this be moved to an issue instead? I thought this was going to be a much simpler fix.

Well, I'm find discussing it here or in an issue, I think the problem is we're not certain what needs to happen yet... which would make an issue more appropriate, but we already have the context here... :-)

I'm whipping up a quick PR for the freeze stuff... it seems mercury is the only misbehaving grammar (at least of those we own)... so I'm still not 100% convinced we need a deep clone.

[joshgoebel]

In the future starting with an issue is probably a better idea. Often the solution you think of isn't actually the correct solution, and having a discussion on an issue first can save time and prevent false starts, etc.

[joshgoebel]

I will pull this back into an issue. I think the correct immediate fix is my PR:

#2271

[joshgoebel]

#2272