Fix crash when declaring function selector of parent class as public constant

[clonker]

Fixes #16050.

I wonder if this can occur for more than just the selector, though. It is somewhat related to PR #11014. Declaring s2 in the test as constant results in it being treated as variable (which has no referenced function entry), declaring it immutable treats it as function and we don't error out. Not sure if we should add some safeguard here.

[cameel]

This is enough as a repro for the issue, but for completeness you should do something with the constant, because we only run the ConstantEvaluator at that point.

E.g. use negation on it.

[cameel]

The test also does not really check if the value of the constant is correct (I'd have to calculate it and compare to be sure). Would be better to have the test verify it:

assert(s2 == B.g.selector);

[cameel]

Suggested change

	bytes4 public constant s2 = B.g.selector;
	bytes4 public constant SELECTOR = B.g.selector;

[cameel]

Not declaring the variable is not necessarily the right solution in a general case. It helps in the repro because we're only accessing the selector, so the variable would go unused, but if we were instead trying to get a pointer to the function, we'd end up trying to access a variable that does not exist.

Theoretically, you should be able to reproduce that by making a constant of an internal function type:

function() constant G = B.g;

But we don't allow this:

Error: Initial value for constant variable has to be compile-time constant.

It actually looks to me like an unrelated bug - I can't see a good reason not to treat references to functions as compile-time constants. It's not like functions can change at runtime.

The weird thing is that the crash happens only when you try to access the selector inside a constant initializer, and not e.g. in the body of a function. You should check why. I'd expect assignInternalFunctionIDIfNotCalledDirectly() to be called in this case too so apparently it does find the ID then. I suspect that the actual cause might be in how (or when) internalFunctionIDs is populated.

[cameel]

We should add some coverage for related cases:

• Using selector in other places outside of functions. E.g.:
  • constructor args when called as a modifier
  • constructor args when called after is
  • immutable initialization
  • state variable initialization
  • file-level constant
  • constant in an unrelated contract or in a library
• Using the reference to the function itself (but still without calling it).

[cameel]

This needs a changelog entry. Also, please triage the issue before starting to work on it :)

[cameel]

I wonder if this can occur for more than just the selector, though.

It does not seem to be related to selector specifically, but .selector happens to be the only thing you can practically do with an internal function in a constant initializer. The fact you can even access selector on it is actually a bit of a special case, because only external functions have selectors. Here the function is accessed via contract name, which makes the compiler see it as an internal function, but as an exception, we do allow accessing selector when we know the function is public. On the other hand you can't do that with address. Rules around that are actually a bit of a mess (#10905).

Anyway, like I mentioned in #16053 (comment), I'd expect problems even when accessing just the function.

It is somewhat related to PR #11014.

Could be. One of the things we use FunctionCallGraph for is to determine which functions should be included in the internal dispatch (which is needed to make internal function pointers work). If the graph does not inspect the constant initializer, it might miss a function being referenced in it.

Declaring s2 in the test as constant results in it being treated as variable (which has no referenced function entry), declaring it immutable treats it as function and we don't error out. Not sure if we should add some safeguard here.

Not sure I understand this bit. What do you meant by it being treated as variable/function?

[cameel]

Looking at the function_selector_access test in the PR you linked, it does have this exact case:

contract C is Base {
	bytes4 constant extConst = Base.ext.selector;
	bytes4 constant pubConst = Base.pub.selector;
}

the only difference being that pubConst is not public, which is apparently another element needed to reproduce the crash.

Still, the call graph does find the call to Base.pub() so constant initializers seem to be properly processed:

{"C", {
	{"Entry", "function Base.ext()"},
	{"Entry", "function Base.pub()"},
	{"function Base.ext()", "function free()"},
	{"function Base.pub()", "function free()"},
}},

Unless the fact that it has a getter affects the graph.