directly parse on/off in pragmas, ignore user override

[metagn]

fixes #22841, fixes #23002

If a pragma like {.checks: off.} or {.warning[ProveInit]: on.} has a direct identifier node on the RHS equal to either on or off, directly parse them as true or false, ignoring user overrides and overloads of the symbols on and off.

Something similar can be accomplished by turning on and off (which are const on* = true and const off* = false in system respectively) into:

type PragmaOption* = enum on, off

and coercing the RHS of pragmas to this type instead of bool (due to new enum type inference), however this would require all code that computes a custom value for the RHS like {.checks: foo().} to change it to something like:

{.checks: (if foo(): on else: off).}
# or
{.checks: PragmaOption(foo()).}

Another problem is the symbols on and off enter into the enum symbol namespace which could cause ambiguity problems in normal code (although if someone has defined enum fields on and off, the pragmas like above wouldn't work anyway).

Either change would require documentation, none is currently written in this PR because it's a POC of the initial proposal above. Will open RFC if required.

[arnetheduck]

oof, this looks like quite the hack :/

[metagn]

This is what I thought would be the least disruptive (I think it makes some sense, it would also be documented). I think the cleanest thing to do would be on and off becoming an enum which only became feasible recently, but this needs slightly more planning and is more likely to break code (though we could alleviate it with a never before seen converter foo(x: bool): PragmaOption {.deprecated.}).

The type narrowing from ambiguous identifiers as you suggested in #23002 (comment) would solve the given issues and seems reasonable in general, but I don't know if we should implement it just to encourage const on = true. It would also take longer, I figured it might be better to deal with this specific case first as multiple people hit this problem recently.

[Araq]

This should simply disambiguate an nkSymChoice to favor the variant that returns bool .

[arnetheduck]

my feeling is that this one-off might be one of those things that stay with us for long and turns out to create subtle incomatibilities when / if this gets a proper fix - type-based disambiguation seems like a pretty natural feature though, could be used in many contexts (ie let v: bool = overloaded()).

Also, I think this is fairly recent which might explain the sudden interest in the issue - I found it when bumping a project from 1.6.12 to 1.6.16.

[metagn]

OK yeah the ambiguous identifier disambiguation is pretty easy, it just needs some annoying code shuffling.

semExpr for nkIdent/nkAccQuoted calls qualifiedLookUp:

Nim/compiler/semexprs.nim

Lines 3039 to 3047 in db9d800

	let checks = if efNoEvaluateGeneric in flags:
	{checkUndeclared, checkPureEnumFields}
	elif efInCall in flags:
	{checkUndeclared, checkModule, checkPureEnumFields}
	else:
	{checkUndeclared, checkModule, checkAmbiguity, checkPureEnumFields}
	s = qualifiedLookUp(c, n, checks)
	if s == nil:
	return

which in turn directly receives a list of the ambiguous identifier candidates (which I didn't know) and just checks if there's more than 1:

Nim/compiler/lookups.nim

Lines 633 to 638 in db9d800

	let candidates = searchInScopesFilterBy(c, ident, allExceptModule)
	if candidates.len > 0:
	result = candidates[0]
	amb = candidates.len > 1
	if amb and checkAmbiguity in flags:
	errorUseQualifier(c, n.info, candidates)

We just need to create a symchoice node from this list and plug it into semSymChoice/fitNode (the symchoice disambiguation logic is obscured in an else branch in paramTypesMatch), but we can only do this outside qualifiedLookup and might have to move it out with an ugly c.ambiguousCandidates = candidates.

[metagn]

Something I should have mentioned/realized is that this, even with the ambiguous identifier resolution:

proc on() = discard

{.warning[ProveInit]: on.}

fails (sorry, proc on works, let on = 123 etc doesn't work). You have to use system.on. This PR or the enum solution don't have this problem.

[Araq]

You have to use system.on.

Then the disambiguation didn't do its job as there is a clear difference between bool and proc ().

[metagn]

Ok, sorry, it does work. My point was that the identifier isn't ambiguous but since it's a proc symbol it calls symChoice anyway (tested with #23123).

This, more expectedly, doesn't work:

let on = 123

{.warning[ProveInit]: on.}

Or const on or for on in ...:, as opposed to importing the on from somewhere else. The question is should we allow it to fail or should on/off in pragmas always work.

[arnetheduck]

should {....: boolReturningCall().} work?

[metagn]

{....: boolReturningCall().}

As found in #23124 which changes the type from bool, someone already has used such syntax. But this PR doesn't affect this, arbitrary expressions outside of on and off are still considered. This PR has other problems, for example expressions like {.checks: (if foo(): on else: off).} will fail when on/off fail to resolve correctly.

I've finished preparing #23123 (the ambiguous identifier resolution suggested above) which fixes the issues, but later down the line if we want to protect against let on = 123 etc we could revisit something like #23124.