Warn if override specifier list order differs from inheritance order

[Marenz]

fixes #8373

[erak]

Looks good already!

[ekpyron]

If we warn about this, then I think we should also adjust the order in the suggestion "Function needs to specify overridden contracts [contracts].". Currently that list will ultimately just be alphabetical.

[Marenz]

that's a good point @ekpyron

@chriseth suggested to use linearizedBaseContracts instead of baseContracts(), however, the following test case creates some issues with that:

for this example, the reversed linearized contracts mismatch

    contract I {
      function f() external virtual {}
    }
    contract A is I {
      function f() external virtual override {}
    }
    contract B is I {}
    contract C is A, B {
      function f() external override(A, I) {}
    }

the reversed list (filtered by the contracts specified in override) is I, A

[ekpyron]

You should check in what order super.f() calls the bases in this case - that's probably the order given by linearizedBaseContracts and that's probably the order we should require the mentions to have. I.e. linearizedBaseContracts is the "correct" order...

[ekpyron]

(the question how super behaves is the core of #8354 which - if I understand correctly - led to the decision to warn about this, so I'd say the required order that's not warned about should be precisely the order of super calls, right?)

[Marenz]

Alright, so it was just my intuition that thought this is wrong then.

[ekpyron]

Alright, so it was just my intuition that thought this is wrong then.

Better actually check what super does - I always found super to be rather weird myself, so I'd need to think about what it really does in this particular case myself :-).

[ekpyron]

FYI I'm myself not actually sure about requiring any particular order for this at all :-). IIUC we still can't guarantee that super.f() will actually call the function in the first mentioned contract, if you inherit from the whole thing again in a weird way...

[Marenz]

Well, one motivation for this was that people thought the order in override(a,b) implies the order of inheritance. If we just require the same as specified at least that particular misunderstanding is eliminated

[Marenz]

updated to use linearizedBaseContracts

[ekpyron]

Some suggestions.

[ekpyron]

Suggested change

	for (size_t i = _item.contract().annotation().linearizedBaseContracts.size() - 1; i > 0; i--)
	{
	auto contract = _item.contract().annotation().linearizedBaseContracts[i];
	if (missingContracts.count(contract))
	sortedMissingContracts.emplace_back(contract);
	}
	for (
	auto const* contract: _item.contract().annotation().linearizedBaseContracts |
	boost::adaptor::filter([](auto const* contract) -> bool { return missingContracts.count(contract); }
	)
	sortedMissingContracts.emplace_back(contract);

?

[Marenz]

Your suggestion is no longer reversing the linearized list?

[ekpyron]

Wouldn't this be much easier, if you did it after you checked that the specified contracts match the expected contracts (and only if they match)?

Then you could just do something like this, couldn't you?

auto overrideIterator = _item.overrides()->overrides().begin();
for (auto const* expected: _item.contract().annotation.linearizedBaseContracts | boost::adaptor::filter([](auto const* base) -> bool { return expectedContracts.count(base); })
  if ((overrideIterator++)->annotation().referencedDeclaration != expected)
  {
     // warn
     break;
  }

[Marenz]

Closing this for now as an in-office discussion suggests we're unsure about this feature