Bugfixes to `builtin` import and tombstone behaviour

[leonerd]

• Make multiple imports idempotent, matching the behaviour of things like strict and feature flags
• Fix various bugs to do with making previously-shadowed symbols visible again when unimporting named builtins

[haarg]

Something that I'm not sure about is the following:

sub true { "true" }

use builtin qw(true);

{
  use builtin qw(true);
  no builtin qw(true);

  print true(), "\n";
}

This will print true, which means the use/no pair didn't return to the previous state.

[leonerd]

This will print true, which means the use/no pair didn't return to the previous state.

Mmmm. Yes that's a fun one indeed. But compare it to features, strict or warnings flags:

$ perl -e 'use feature "say"; { use feature "say"; no feature "say"; say "Boo" }'
String found where operator expected (Do you need to predeclare "say"?) at -e line 1, near "say "Boo""

$ perl -ce 'use strict; { use strict; no strict; @{"main::ISA"} = (1); }'
-e syntax OK

$ perl -e 'use warnings; { use warnings; no warnings; 1 + undef }'

In each of these cases, the things are simple on/off switches, they don't have activation counts. One no is enough to undo within that scope the effect of any number of preceeding uses.

It's hard to correctly see how to simultaneously make both of these tests pass:

sub true { "yes" }
use builtin 'true';  # <-- THIS LINE
{
  use builtin 'true'; no builtin 'true';
  is( true(), "1", 'true is builtin' );
}

sub true { "yes" }
{
  use builtin 'true';  # <-- THIS LINE
  use builtin 'true'; no builtin 'true';
  is( true(), "yes", 'true is package sub' );
}

The only thing different is the scope position of the marked line. Locally from the perspective of the is() call, the same things are visible in its scope in both cases, so why should it see two different behaviours.

A question we have to ask is are both of these tests correct? Or is only one of them correct and the other should fail? Which one?

By comparison to feature / strict / warning flags, it is at least consistent to say that lexical imports of builtins don't have a counting behaviour, and thus multiple activations can be cancelled by a single deäctivation. It may not be exactly what is expected in all cases, but it's the same as the other things that we already have.

[happy-barney]

re multiple use builtin 'true' ... behaviour can be considered wrong from two points of view:

• second use should produce warning redefine if this usage of use is stackable (ie, treating as a function)
• second use should be noop if use treated as declaration of expected state

From my perception of programming, use <pragma> should be treated as declarative state of what is expected how language should behave in scope of validity of given pragma.

[haarg]

In each of these cases, the things are simple on/off switches, they don't have activation counts. One no is enough to undo within that scope the effect of any number of preceeding uses.

I think "no builtin" only really makes sense if we think of it as just removing a lexical. This means it should result in either the previous lexical being unmasked or no lexical being available. So this is in a way "counting".

It's hard to correctly see how to simultaneously make both of these tests pass:

sub true { "yes" }
use builtin 'true';  # <-- THIS LINE
{
  use builtin 'true'; no builtin 'true';
  is( true(), "1", 'true is builtin' );
}

sub true { "yes" }
{
  use builtin 'true';  # <-- THIS LINE
  use builtin 'true'; no builtin 'true';
  is( true(), "yes", 'true is package sub' );
}

The second of these should fail. true should also be returning "1". And it should also issue a warning about masking an existing lexical sub.

use builtin 'true'; should be exactly equivalent to my sub true () { !!1 }. This means that doing it multiple times in the same scope should warn. That would also mean use v5.40; use builtin 'true'; would warn.

If that seems wrong, then I think lexical exports are the wrong tool for this.

[leonerd]

This entire PR and the wider "tombstone" mechanism are now being abandoned, in favour of the simpler model of "you can't get rid of lexicals". Which means use VERSION will have to interact slightly differently with builtin bundles. This is the new model implemented by #22002.

Once that and the removal of builtin::unimport have been applied, this PR can be closed.

[jkeenan]

1. For those who haven't been following along ... what is the "tombstone" behavior that is being eliminated? That term doesn't appear in our documentation.
2. Branch now has merge conflicts.

[leonerd]

1. For those who haven't been following along ... what is the "tombstone" behavior that is being eliminated? That term doesn't appear in our documentation.

It's all really new, having just been created to attempt to implement no builtin ..., which we're about to remove anyway. A short-lived experiment that has already been decided failed.

2. Branch now has merge conflicts.

Yup but don't worry, I'm going to delete it anyway.

[happy-barney]

@leonerd preventing multiple use VERSION will break possibility gradually upgrade parts of code.

Why to even install builtins as functions? wouldn't it be easier to:

• use builtin X will just turn on flag for X
• toke.c will return X as keyword only if flag for X is enabled

(I already posted poc for data structure for "unlimited" number of feature flags ~3 years ago - https://pastebin.com/Lnuq3Nec)

[leonerd]

This can all be abandoned now. We've already removed all the tombstone related logic from builtin and use VERSION, and #22063 will remove the entire concept.