Check for consistency of fixed-point precision in operations

[ISSOtm]

Goal: print a warning when doing any of these

• 1.0 + 1
• 1.0 * 2.0
• MUL(1.0q8, 2.0q8, 16)
  • Check that we still warn if q8 and/or , 16 are omitted (with the appropriate OPT Q setting, of course).
• 1.0q8 + 2.0q16
• Warn if any fixed-point value is passed to RGBLINK, as it lacks precision info (and I don't think it's worth adding, fixed-point ops are clearly meant to be kept at compile time).

We may want that warning to have levels, since e.g. someone may be intentionally multiplying fixed-point numbers together and correctly dealing with the resulting precision change.

Bikeshed points:

• How many levels to have?
• What level to assign to each case?
• Which levels should be default, assigned to -Wall, assigned to -Wextra?

(Originally brought up in #1455 (comment))

[Rangi42]

I strongly disagree with this.

It would require the parser to inspect the lazy-evaluated RPN expression, and care about the structure of those expressions.

It's a problem without a definite stopping point for a solution, because expressions can nest and expand indefinitely. Should we warn about MUL(DIV(12.0q16, 6.0q16), DIV(9.0q8, 3.0q8))? (So fixed-point functions would have to track the "Q-ness" of the result?) Should we warn about 1.0q8 + N when it's DEF N EQU 2.0q4, or DEF N EQU $200, or DEF N EQU GLACEON? Or how about bit 1.0q2, [hl]?

This is basically a request for RGBASM to have actual data types. Not just "numbers" and "strings", but "integer" and "fixpoint", and probably "constant" and "variable" as well.

Perhaps you can design a GB assembly language from scratch to have typed values, and manage to have them be more useful than annoying, but I don't think we can or should tack them on to the language we have.

[aaaaaa123456789]

I've done 1.0 + 1 deliberately many times. This is actually useful.

[ISSOtm]

I thought this could be implemented without too much hassle by making numbers in RGBASM a { value: u32, precision: u8 }. (Integers simply are q0.) This requires no changes outside of adding checks to the expression evaluator, and the function that converts a constant expression into a RPN buffer.

Unless that implementation strategy has obvious holes, I think it's beneficial because it doesn't add any concept of data types beyond what we already have, while adding diagnostics that we'd normally get only from type checking.

[Rangi42]

I don't see any principled way to draw the line there, though. If we alert the user in this instance, why not warn them about 2 + &10 (mixed bases can be just as bad as mixed precisions!), or bit 2 * 3, a (when is it sensible to calculate a bit index by multiplying?), or --N (double negation is silly, they must have wanted to decrement!)?

I'd much rather draw the line where it currently is, at "numbers are numbers, we provide lots of syntaxes meant for different circumstances, pick the ones that work for you".

[ISSOtm]

I don't see a principled way either, it's just that I've found it somewhat tedious to have to keep thinking about that info as I've been manipulating fixed-points (e.g. to check the correctness of the correctness checker below ↓), and would've appreciated the tooling to “have my back” to catch mistakes more easily.

assert (HIGH(NB_TILE_ROWS_KEPT_LIVE * 8.0) - 1) & HIGH(wMapTiles << 1) == 0 ; As you can see, these two don't overlap. (And note that the first one is entirely contiguous.)

I've done 1.0 + 1 deliberately many times. This is actually useful.

That same file contains an instance of <fixed-point> - 1, so I'm far from disagreeing; but IMO, OPT W/PUSHO W is there to briefly opt out of the warning for known-good operations, and still have a guardrail everywhere else.

[Rangi42]

I also think that just having a concept of "numbers know their own precision" strongly invites further changes that are at odds with how we've made the language so far.

For example, if you do MUL(2.0, 3.0) then it uses the default precision -- but is that because of the current context which sets that precision, or because the two values got their precisions from the context? So if you do MUL(2.0q8, 3.0q8) instead, why should that do the "obvious" thing and act like what MUL(2.0q8, 3.0q8, 8) does now? (So those optional precision args are now pretty useless; do we deprecate them?)

And hey, if numbers are carrying metadata around anyway, why limit ourselves to definition time? If I do bit FOO, a, then FOO could know that it's a "bit constant" -- and warn if I do DEF FOO += 10 because now it's out of the 0-7 range. Or $FF00FF00 could be tagged as "unsigned" while -16711936 is tagged as "signed" (and yet $FF00FF00 == -16711936; shall we copy gcc's -Wsign-compare?)

[aaaaaa123456789]

If you want a static checker, that belongs in a separate tool, not in RGBASM's default configuration. It's an assembler, not Rust.

[Rangi42]

(Can an assembly language be more like Rust? I'd like to see one, that could be cool! But RGBASM isn't.)

[aaaaaa123456789]

(Can an assembly language be more like Rust? I'd like to see one, that could be cool! But RGBASM isn't.)

There is no reason why you can't make a static checker. You can probably even move part of the 1.0 code into a library and use that as a starting point. More tooling is always a net positive, as people can easily choose if the extra tool fits their needs or not.

The issue is when that extra tooling stops being extra tooling and becomes part of the language — then the tool is no longer a bonus feature and it becomes a shackle for those who don't need it.

[Rangi42]

(Maybe it's something to reconsider with the Rust rewrite, when we can more easily factor the lexer+parser out into a reusable crate for both the assembler and checker. (And even things like optimize.pyrs!))

[ISSOtm]

The issue is when that extra tooling stops being extra tooling and becomes part of the language — then the tool is no longer a bonus feature and it becomes a shackle for those who don't need it.

That's arguably true of many warnings-as-lints that we have, such as -Wtruncation, so I'm unconvinced.

(Can an assembly language be more like Rust? I'd like to see one, that could be cool! But RGBASM isn't.)

Took me a while, but I found it!

(Maybe it's something to reconsider with the Rust rewrite, when we can more easily factor the lexer+parser out into a reusable crate for both the assembler and checker. (And even things like optimize.pyrs!))

I do plan to have at least an AST export, being basically a prerequisite to a LSP. But that's way far out.

[aaaaaa123456789]

That's arguably true of many warnings-as-lints that we have, such as -Wtruncation, so I'm unconvinced.

Why do you think these warnings have caused so much trouble?

[ISSOtm]

I have been repeatedly helped by -Wtruncation catching some fixed-point manipulation mistakes in this past week, so I have zero idea what ”so much trouble” may be referring to.

[aaaaaa123456789]

I have been repeatedly helped by -Wtruncation catching some fixed-point manipulation mistakes in this past week, so I have zero idea what ”so much trouble” may be referring to.

The person wanting more static checkers being helped by static checkers should surprise exactly nobody. You'll have to look at people other than you (and people with coding practices similar to yours) if you want to see people being hindered by static checkers. I personally ran into -Wtruncation issues many, many times before the two separate limits were added, and I told you so, but apparently that doesn't count for some reason...

[ISSOtm]

I'm aware of your style, but that makes one example on each side of the balance, so AFAICT no definite sway. I lack more data points to make an informed decision—and, in the absence of that, can you blame me for putting in effort to push the tool in a direction that I am more comfortable with?

I don't like breaking out that argument, but I haven't seen people PRing default warning flags demotion, or discussing the usefulness of existing warnings with examples (text and/or screenshots, we're not picky); so please forgive me if I sound a little miffed.

[aaaaaa123456789]

I didn't make a PR for the -Wtruncation limits because you explicitly forbade me from doing so. I was about to make it when you said that.

As I said, the happy medium that makes both sides of the debate happy is to separate the checks into other tools, so people can integrate the tools/checks they want without being forced into them. I'm not saying those tools can't exist. Just don't shove them down users' throats!

[Rangi42]

My data point doesn't shift the balance either, because I'm somewhere in the middle. :P

I appreciate the safety net of -Wtruncation=1, and IIRC helped implement or at least argue for it.

But I do specifically set =1 not =2, because I expect to be able to bit-negate any valid N-bit value and have it stay within N bits. Even if it's technically a negative number outside the range, ld a, ~(1 << FOO_BIT) when FOO_BIT == 7 is useful, and ld hl, -wLabel is useful.

[aaaaaa123456789]

(Sidenote: I've personally written ld a, 400 deliberately. But since that's a one-off, a single instance of ld a, LOW(400) doesn't bother me too much.)

EDIT: I lied, it was sub 400...

[Rangi42]

As someone who might have to read or edit that code, I very much appreciate the explicit LOW so I don't have to inspect the context to see if you just typo'ed 40. :)

[ISSOtm]

As I said, the happy medium that makes both sides of the debate happy is to separate the checks into other tools, so people can integrate the tools/checks they want without being forced into them. I'm not saying those tools can't exist. Just don't shove them down users' throats!

Considering that the tool that the warning originates from is irrelevant, it seems to me that what you're really asking is for the warning to become opt-in instead of opt-out. (The only difference is that it's either opt-in by running rgblint, or by passing -Wfixpoint-precision=42.)

In that context, I don't see why the warning cannot be implemented at all, and the bikeshedding then shifts to “what's a good default”, “what are good preset levels”.

[ISSOtm]

But I do specifically set =1 not =2, because I expect to be able to bit-negate any valid N-bit value and have it stay within N bits. Even if it's technically a negative number outside the range, ld a, ~(1 << FOO_BIT) when FOO_BIT == 7 is useful, and ld hl, -wLabel is useful.

For that matter, I've hit a and ~$80 (it was a slightly more complex expression, but you get the point) once during the past week, for I think the first time since I've started maintaining RGBDS. I just added a LOW() and went on with my day—as far as I'm concerned, it is a one-off.

[Rangi42]

And I have never dealt with negative numbers in the precise range where =2 would have caught some mistake. ld a, ~(something with the bit 7 set) is pretty common IME, or at least commonly possible, as you write something like ld a, ~DEFAULT_FLAGS and at some point the flags get 7 set. But ld a, -129 just doesn't happen.