Small changes for correctness by ShawSumma · Pull Request #372 · temperlang/temper

ShawSumma · 2026-03-13T15:37:43Z

I used the AI to identify and fix small problems in the backends, mostly focusing on correctness, but also doing some inlining.

Signed-off-by: Shaw Summa <shawsumma@gmail.com>

…fixes-rust

mikesamuel · 2026-03-14T23:25:55Z

Could you add to functional-test-suite/src/commonMain/resources/types/float/ops/ ?

That'd make sure the corner cases are exercised for all backends.

notactuallytreyanastasio · 2026-03-19T15:12:48Z

Worth noting that in theory this solves #373 in theory and maybe should add a specific test there. Leaving this as a note so its linked in GH history.

tjpalmer

Some of these changes are good, some are meh, some are bad, and some I still haven't looked at enough.

If you have different opinions on any, let me know.

Minimally, this maybe can serve as an example of why not to just do everything that AI says. Which should help for some talking points of why Temper still matters.

tjpalmer · 2026-03-19T16:48:52Z

            var abs = absTol ?? 0.0;
            var margin = Math.Max(Math.Max(Math.Abs(x), Math.Abs(y)) * rel, abs);
-            return Math.Abs(x - y) < margin;
+            return Math.Abs(x - y) <= margin;


This does match Python's math.isclose better. I'll plan to see if everything else also gets updated in this pr.

tjpalmer · 2026-03-19T16:52:31Z

                : trimmed == "-Infinity"
                    ? double.NegativeInfinity
-                    : double.Parse(s);
+                    : double.Parse(trimmed);


In Remarks here, it says that double.Parse allows for surrounding whitespace, so it doesn't matter much whether s or trimmed goes in. Maybe worth a comment either way, and maybe trimmed is less surprising. So probably an ok change to make, even if s also works.

tjpalmer · 2026-03-19T16:53:48Z

            {
                count += 1;
-                if (i + 1 < right)
+                if (i + 1 < limit)


Because the loop prevents i from going past limit, either works fine here, so again, a needless change but also not a breaking one. And maybe limit is less surprising to some people.

tjpalmer · 2026-03-19T16:55:19Z

            {
-                return -1;
+                return i;
            }


I personally feel a lot better always using -1 for no string index rather than arbitrary negative values. Is there a good reason for this change that I'm missing? If not, I'd like to revert this.

I agree with Tom on this.

tjpalmer · 2026-03-19T16:55:41Z

  }
  const margin = Math.max(Math.max(Math.abs(x), Math.abs(y)) * relTol, absTol);
-  return Math.abs(x - y) < margin;
+  return Math.abs(x - y) <= margin;


So we've updated cs and js so far.

tjpalmer · 2026-03-19T17:33:39Z

            val calleeTree = callToInline.definition
            val formalNameToArg = callToInline.formalNameToFunArg
            val thisBindings = callToInline.thisTypeBindings
-            callToInline.thisTypeBindings


This looks like good cleanup at a glance.

tjpalmer · 2026-03-19T17:34:35Z

                else -> type
            }
-            is OrType -> type.members.firstNotNullOf { findNominal(it) }
+            is OrType -> type.members.firstNotNullOfOrNull { findNominal(it) }


I haven't analyzed to know if this is a good change or not.

tjpalmer · 2026-03-19T17:36:02Z

                        // larger shift or comparison operator, or close tag start marker.
                        val before = text[end - 1]
-                        val after = if (end + 1 < limit) { text[end - 1] } else { '\u0000' }
+                        val after = if (end + 1 < limit) { text[end + 1] } else { '\u0000' }


The old code looks sus, but I don't know why it wouldn't have triggered problems. No test changes from this?

Nope, no changes, it's one of those "how did it ever work?!" things.

tjpalmer · 2026-03-19T17:37:06Z

    fun append(str: String) { buffer.append(str) }

+    @Synchronized
    override fun toString(): String = "$buffer"


Again, some analysis here would be good, which I haven't done right now.

It makes sure the buffer is never read at the same the above append is happening, as that would be bad and give you a toString() that never actually existed.

It makes sure the buffer is never read at the same the above append is happening, as that would be bad and give you a toString() that never actually existed.

I guess the question is if it's ever used from multithreaded contexts. I still haven't looked, though.

For the time being it feels like it could be, as the @Synchronized on the other method is preexisting. If it weren't used multithreaded then both should be removed.

Signed-off-by: Shaw Summa <shawsumma@gmail.com>

tjpalmer · 2026-03-19T18:45:18Z

Could you add to functional-test-suite/src/commonMain/resources/types/float/ops/ ?

That'd make sure the corner cases are exercised for all backends.

Maybe a corner case test for near? Maybe also some large int64 cases that might trigger the old Python int things? I already forget if other additional testing would be good.

Signed-off-by: Tom <tom@contextfree.info>

tjpalmer · 2026-03-25T00:11:00Z


 function temper.string_indexof(str, target, i)
-    return string.find(str, target, i, true) or 0
+    return string.find(str, target, i, true) or -1


For lua, I think we're using 0 as no string index, since indices start at 1?

I liked having it be -1 everywhere, tho maybe this needs more thought

Does this affect == with NoStringIndex

Does this affect == with NoStringIndex

I put an approve, but maybe this is worth testing somewhere sometime if we don't yet.

tjpalmer · 2026-03-25T16:17:24Z

-                TmpLOperator.AmpAmp -> TODO() // RustOperator.LogicalAnd
-                TmpLOperator.BarBar -> TODO() // RustOperator.LogicalOr
+                TmpLOperator.AmpAmp -> RustOperator.LogicalAnd
+                TmpLOperator.BarBar -> RustOperator.LogicalOr


I see here's where they were being ignored before. Looks like we don't actually translate to using these. Instead we assign bool values from if/else sequences. But probably good to implement these anyway, just in case, and even if not easy to test, the implementations look ok.

* Maybe address RustTranslator concerns * Take out redundant rust things, also mapDropping Signed-off-by: Tom <tom@temper.systems>

tjpalmer

There's still a lot of complicated stuff here, but I'm ready to call it good enough, anyway. Maybe we can restrict the number or variety of AI changes in single PRs in the future some.

I have some old pending comments here I also forgot to submit earlier, but meh.

tjpalmer · 2026-03-25T18:53:35Z

+            }
+        }
+        Arc::new(result)
+    }


We got by without it because nothing has ever tested mapDropping. The above is also incorrect, because it's based on Option instead of Result. I'll back out the mapDropping changes from this branch, because it would increase scope. At least be-csharp also doesn't implement mapDropping. I haven't tested all backends yet.

tjpalmer · 2026-03-25T18:57:11Z

+    }
+}
+
+private val print: SupportCode = PrintCode


Yeah, print also is a relic that we no longer produce. Not sure about the other compares, but as they're not being used, I'm feeling too busy to study and remove them from this pr. Just leaving this comment here, so people can know that they haven't ever really been tested.

ShawSumma added 9 commits March 9, 2026 20:23

ai

60e6da2

Signed-off-by: Shaw Summa <shawsumma@gmail.com>

ai fixes to type stuff

4b1b76d

Signed-off-by: Shaw Summa <shawsumma@gmail.com>

ai fixes for rust

d71c325

Signed-off-by: Shaw Summa <shawsumma@gmail.com>

ai fixes for rust

00660a4

Signed-off-by: Shaw Summa <shawsumma@gmail.com>

ai fixes for lua

8d42701

Signed-off-by: Shaw Summa <shawsumma@gmail.com>

ai fixes to rust

8eaf9fd

Signed-off-by: Shaw Summa <shawsumma@gmail.com>

ai fixes to lua

f06fc9f

Signed-off-by: Shaw Summa <shawsumma@gmail.com>

Merge remote-tracking branch 'origin/main' into shaw-ai-fixes-rust

0ec6083

Merge remote-tracking branch 'origin/shaw-ai-fixes-lua' into shaw-ai-…

4137463

…fixes-rust

ShawSumma marked this pull request as ready for review March 14, 2026 11:57

tjpalmer mentioned this pull request Mar 14, 2026

Temper has Float64 division that generates NaN / Infinity in Python backend #373

Open

tjpalmer reviewed Mar 19, 2026

View reviewed changes

revert things as per tom's suggestions

94d7e19

Signed-off-by: Shaw Summa <shawsumma@gmail.com>

Finish changing and testing near maybe (#386)

368455d

Signed-off-by: Tom <tom@contextfree.info>

tjpalmer reviewed Mar 25, 2026

View reviewed changes

Clean up rust changes (#395)

e29d393

* Maybe address RustTranslator concerns * Take out redundant rust things, also mapDropping Signed-off-by: Tom <tom@temper.systems>

tjpalmer approved these changes Apr 3, 2026

View reviewed changes

ShawSumma merged commit fb9a909 into main Apr 3, 2026
2 checks passed

Conversation

ShawSumma commented Mar 13, 2026

Uh oh!

mikesamuel commented Mar 14, 2026

Uh oh!

notactuallytreyanastasio commented Mar 19, 2026

Uh oh!

tjpalmer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tjpalmer commented Mar 19, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tjpalmer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants