CommonMethods: declarative specification of methods

[GreyCat]

• Refactored CommonMethods to use declarative specification of methods, and build all checks for arguments quantity and types around that, equally for all the data types and implementations.
  • Introduced set of new case objects derived from MethodArgType to be able to embody expectations about argument types (e.g. case _: IntType will be represented as IntArg).
  • Introduced set of new MethodSig classes to be able to declaratively specify method signatures.
• Added ExpressionValidator$Test that will test that functionality (as ExpressionValidator will be first in line, resuing CommonMethods functionality)
• Added a few new exceptions to clearly communicate CommonMethods checks results (WrongMethodCall, MethodNotFoundErrorWithArg).
• ExpressionValidator: fixed problem with validation of subscripts usage (and added relevant test to ExpressionValidator$Test).
• Fixed enum warning -> enumName for Scala 3 compatibility.

[generalmimon]

To begin with, I just have to make one technical remark: please resist the urge to write "nameless" commit messages like the one in 580e4bc. I mean the detail of the content is fine, but the problem is that it's missing the title line, so Git tools don't have a good way to display this. GitHub truncates it after the first line break (which means that I only get to see the beginning of your first bullet point), but my usual way of displaying the commit tree locally doesn't, which is arguably even worse:

Needless to say, if you feel the urge to cram so many bullet points into one commit, it should've likely been multiple commits. But you probably know this ;)

---

@GreyCat I strongly recommend you to check out https://cbea.ms/git-commit/#seven-rules. The relevant and the most important rule in my opinion is https://cbea.ms/git-commit/#separate - "Separate subject from body with a blank line".

Another good rule is to keep the subject line short, ideally under 50 characters - https://cbea.ms/git-commit/#limit-50. I personally try to follow this, so I know very well that 50 characters is often not much for what I want to tell the world, so I often allow myself to go with 65 or so. Just like they recommend:

So shoot for 50 characters, but consider 72 the hard limit.

[generalmimon]

@GreyCat Thanks for this improvement! The code looks good to me, but I haven't tested it - hopefully it works as expected 🤞

[GreyCat]

To begin with, I just have to make one technical remark: please resist the urge to write "nameless" commit messages like the one in 580e4bc.

Hey, really appreciate you calling this out. Much to my shame, I was not really aware of this integration capabilities, nor I have ever pain attention to this. Will fix.

[generalmimon]

I've just noticed that returnType is not used anywhere and the detectAttributeType method still contains match cases like case "first" | "last" | "min" | "max" => Int1Type(false), I suppose the detectAttributeType method is planned to be migrated to use these new method signatures (in this PR / later)?

[GreyCat]

Great observation! Yeah, that is the idea. Or at least that was the idea.

Now, though, the more I think about it, the more I understand how our story with all these checks/modularization went crazy and it's hard to untangle it:

• TypeProvider is keeping state of current parsing point for resolving fields out of "this" context, providing point in the tree, and keeping KS io.kaitai.struct.format constructs separate from io.kaitai.struct.exprlang. We can argue how bad it is that it's stateful, but for now I believe this is least of the concerns.
• TypeDetector existing by itself (and used in quite a few places, including CommonMethods) — but ideally this should be tied closely to a declarative method description like the one in CommonMethods, otherwise we have a lot of duplication and error stemming from that.
• CommonMethods[T] and other generic components that together make up a "translator" — but they for sure depend on TypeDetector (or do they?)
• ExpressionValidator which follows "translator" pattern in spirit (by providing def validate(v: Ast.expr): Unit which throws exceptions), but not in letter (i.e. it doesn't inherit from AbstractTranslator or BaseTranslator, because their basic requirement is having def translate(v: Ast.expr): String).
  • It still inherits TypeDetector.
  • It still inherits CommonMethods[Unit].
  • In the hindsight, I see the idea of returning Unit and throwing exceptions as particularly bad — it should have been refactored as returning list of CompilationProblem or something like that, so we can accumulate those.
• TypeValidator (which is a precompile step) traverses everything and then selectively invoked ExpressionValidator on expressions to both use it as TypeDetector and do validate(...). This one also adds layer of exception catching wrappers (which are, in all fairness, done quite poorly) and do that translation from exceptions to CompilationProblem.

So, a lot of things to think of...