Reports an error on unimplemented modifiers in a function definition.

[christianparpart]

Partial fix for #11468

(some tests are still failing though. working on it).

I'll squash commits once 👍'd

[chriseth]

So your fix is to require an overriding modifier to be implemented, right? Is this the same requirement for functions? If yes, can we somehow to do it in a uniform manner?

[cameel]

@chriseth For functions the rule seems to be that you cannot override an implemented function with an unimplemented one unless:

• The function you're overriding is unimplemented too.
• You use multiple inheritance with a linearized order that effectively puts the unimplemented function before the implemented one (this is basically ICE when invoking a modifier overriden by an unimplemented one #11468).

If you manage to override it, you can call such an unimplemented function it in two cases:

• There is no code actually generated for the call. E.g. because you do it in a function that is itself overriden.
• (after Make super skip unimplemented functions #11472) You make a call via super and the unimplemented function gets skipped.

@christianparpart Looks like this breaks quite a few tests. I think that we need to allow at least cases where the modifier is used in its own (abstract) contract but only implemented in one that inherits from it (empty_modifier_body.sol). Those are probably not the only ones though.

Also, you should add some new tests, including the one from the issue. I'd also add some where you explicitly reference the unimplemented modifier via its contract name (we do not have a test for that).

[cameel]

I think we could use some positive test cases too (unless they already exist somewhere):

Overriding unimplemented with unimplemented:

abstract contract A {
    modifier m() virtual;
}
abstract contract B is A {
    modifier m() virtual override;
}
abstract contract C is B {
    modifier m() virtual override;
    function f() m public {}
}

Leaving a modifier somewhere in the chain unimplemented through multiple inheritance:

contract A {
    modifier m() virtual { _; }
}
abstract contract B {
    modifier m() virtual;
}
contract C is A, B {
    modifier m() override(A, B) { _; }
    function f() m public {}
}

Use of an unimplemented modifier in an abstract contract:
(EDIT: Replaced A.m with m. Also, this should not be an error after all)

abstract contract A {
    modifier m() virtual;
    function f() m public {}
}
contract B is A {
    modifier m() virtual override { _; }
}

Use of an unimplemented modifier on a method that gets overridden:
(EDIT: Replaced A.m with m, changed B to abstract and removed m override from B)

abstract contract A {
    modifier m() virtual;
    function f() m public virtual {}
}
abstract contract B is A {
    function f() public override {}
}

[cameel]

What about these? Could you add them as tests?

All but the second one are currently errors (Cannot call unimplemented modifier.). I think they should all work without errors because equivalent examples for virtual functions do pass.

[chriseth]

I think this depends on whether the lookup is virtual or static.

[cameel]

Most of the comments below are cosmetic issues but there's also one that's a blocker in my opinion: I think that the cases I listed in #11471 (comment) are not handled correctly. They result in errors in cases that worked before and should still work because they work for functions.

[cameel]

This could be moved into the if below.

[cameel]

Suggested change

	if (m_currentContract->derivesFrom(*modifierDecl->annotation().contract) &&
	!modifierDecl->isImplemented()
	)
	if (
	m_currentContract->derivesFrom(*modifierDecl->annotation().contract) &&
	!modifierDecl->isImplemented()
	)

[cameel]

Suggested change

	m_errorReporter.typeError(
	9428_error,
	_modifier.location(),
	"Can only use modifiers defined in the current contract or in base contracts."
	);
	m_errorReporter.typeError(
	9428_error,
	_modifier.location(),
	"Can only use modifiers defined in the current contract or in base contracts."
	);

[cameel]

Isn't this really equivalent to contains(m_currentContract->annotation().linearizedBaseContracts, modifierContract) used above? I think this if could be just else for the if above, with the !modifierDecl->isImplemented() check inside.

[cameel]

We typically don't include these in tests. They get added automatically by the test suite.
And why specifically 0.5.0? The B.m call syntax for modifiers wasn't added until 0.7.x.

[cameel]

What about these? Could you add them as tests?

All but the second one are currently errors (Cannot call unimplemented modifier.). I think they should all work without errors because equivalent examples for virtual functions do pass.

[chriseth]

Needs test updates.

[cameel]

I have changed the first comment because now this no longer completely fixes #11468.

[chriseth]

I think @christianparpart wanted to create a new issue for the other part.

[cameel]

I think it would be better to still include test cases from #11471 (comment) in this PR. If the PR changes the behavior for any of them, then it would be a non-backwards compatible change.