Add unary "+", "-" and "!"

[mns-nordicals]

Fixes #52

Not-operator only works with logical variable. If you want, I guess it could be extended to integer and double where 0 and 0.0 is true and false otherwise.

[mns-nordicals]

@codex review (can I also do it ?)

[chatgpt-codex-connector]

Codex Review: Here are some suggestions.

About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".

[chatgpt-codex-connector]

[P1] Avoid reusing operand Variable in unary handlers

The new unary +/-/! handlers return Fortran(..., x@value) which reuses the same Variable object as the operand. When that result is assigned to a different symbol (e.g. y <- -x) the assignment code renames x@value to y, so the original binding for x no longer has a declaration. Any later reference to x in the function will emit x in the generated Fortran but no corresponding Variable exists, producing undeclared variable errors at compile time. Please create a fresh Variable(x@value@mode, x@value@dims) for unary results instead of mutating the source. The same issue appears in the unary - and ! handlers below.

Useful? React with 👍 / 👎.

[mns-nordicals]

@codex please review again

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Nice work!

About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".

[mns-nordicals]

Hmm ... after trying to understand codex suggestion, I think I have come to the conclusion, that it is not necessary. Codex seem to think S7 works like R6 in this case.

I think this part was confusing it in the assignment handler ["<-"]

  # immutable / copy-on-modify usage of Variable()
  if (is.null(var <- get0(name, scope))) {
    # this is a binding to a new symbol
    var <- value@value
    var@name <- name
    scope[[name]] <- var

Even though the comment state it is immutable /copy-on-modify.

[t-kalinowski]

Looks great! Can you please add a NEWS entry?

[mns-nordicals]

Reverted last commit and added news entry.

[t-kalinowski]

Thanks!

Looking at the code again just now, I see that the Codex suggestion was right, just for the wrong stated reason!

A Variable does have copy-on-modify semantics, so we don't have to worry about modifying in place. However, passing it forward in a handler is not a good pattern, because the Variables have a @name property! Since the result of this expression is an anonymous object, we should create new Variable which does not contain an incorrect @name (ditto for the @r property).

[mns-nordicals]

We can revert back again. As I understand it, in the assignment handler around here:

  stopifnot(is.symbol(target))
  name <- as.character(target)

  value <- args[[2]]
  value <- r2f(value, scope, ...)

  # immutable / copy-on-modify usage of Variable()
  if (is.null(var <- get0(name, scope))) {
    # this is a binding to a new symbol
    var <- value@value
    var@name <- name
    scope[[name]] <- var
  } else {

We take the symbol-name on the left side (y <- x) and then we replace the copied x-name with the target y. And this is what saves us.

If we fixed it here, I think we also need to fixed in the other unary intrinsic section. e.g

r2f_handlers[["abs"]] <- function(args, scope, ...) {
  stopifnot(length(args) == 1L)
  arg <- r2f(args[[1]], scope, ...)
  Fortran(glue("abs({arg})"), arg@value)
}

We are using the same pattern.

[t-kalinowski]

Oh, good catch! That should definitely be updated in abs and similar handlers.

[mns-nordicals]

I have previously tried to create a bug related to this Variable copying, but I couldn't come up with one. I aasked codex to try and create a bug based on this conversation and it found something, but I am not entirely sure that it is related and it is not about casting logical.

Perhaps we can merge this one and then I don't mind creating a new PR for more robust handling of this and other handlers.

Here is what codex had to say:

Unary handler Variable reuse causes compile-time error

Summary: Reusing the operand’s Variable object in unary handlers (e.g., !, unary +/-, and abs) carries identity fields like @name, @is_arg, and @r into the anonymous result. This can mark a local result as “external” and trigger external-argument casting logic later, producing invalid Fortran and a compiler error.

Reproduction

• Minimal function that negates a logical input into a local and uses it as a mask:

devtools::load_all(quiet = TRUE)

qfun <- quick(function(x){
  declare(type(x = logical(NA)))
  y <- !x
  out <- ifelse(y, 1L, 0L)
  out
}, name = "bug_unary_not_local_cast")

qfun(c(TRUE, FALSE, TRUE))

• Compiler error (gfortran):

bug_unary_not_local_cast_fsub.f90:19:33:

   19 |   out = merge(1_c_int, 0_c_int, (y/=0))
      |                                 1
Compiler Error: Operands of comparison operator ‘/=’ at (1) are LOGICAL(4)/INTEGER(4)

What was generated and why it’s wrong

• The symbol emission logic converts external logicals to a boolean via integer compare:

  • R/r2f.R:90 emits (s/=0) when a logical symbol is marked external (@is_external, i.e., arg/return).

• The unary not handler returns the operand’s Variable as-is:

  • R/r2f.R:743–R/r2f.R:750 uses Fortran(glue("(.not. {x})"), x@value).

• Assignment of y <- !x binds a new symbol using that same Variable instance:

  • R/r2f.R:867–R/r2f.R:901 (new-symbol case) does var <- value@value; var@name <- name; scope[[name]] <- var.

• Because x@value had @is_arg == TRUE (it’s an input argument), the new local y inherits @is_arg == TRUE via reuse. That makes y look external, so when y appears later as a symbol, the symbol logic emits (y/=0). But y is a local logical (not an integer), hence the invalid LOGICAL/INTEGER comparison seen in the compiler error.

Where the pattern appears

• Unary logical not:
  • R/r2f.R:743–R/r2f.R:750
• Unary plus/minus:
  • R/r2f.R:660–R/r2f.R:669 (+)
  • R/r2f.R:671–R/r2f.R:680 (-)
• abs (two definitions):
  • R/r2f.R:542–R/r2f.R:546
  • R/r2f.R:586–R/r2f.R:593 (complex → double)
• Other unary elementals (sin, cos, tan, asin, acos, atan, sqrt, exp, log, floor, ceiling) also forward arg@value:
  • R/r2f.R:552–R/r2f.R:572

Root cause

• Reusing arg@value for anonymous unary results forwards identity fields (@name, @is_arg, @is_return, and @r).
• The symbol emission special-case for external logicals applies incorrectly to locals that have inherited @is_arg.

Fix

• In all unary handlers, create a fresh Variable for the result with the correct mode and dims, rather than forwarding arg@value:
  • Use Variable(arg@value@mode, arg@value@dims) (or appropriate mode override) and pass that as the Fortran value.
  • This avoids inheriting @name, @is_arg, and @r.

Concrete changes

• Logical not:

  • R/r2f.R:743–R/r2f.R:750
  • Replace:

    Fortran(glue("(.not. {x})"), x@value)

    With:

    Fortran(glue("(.not. {x})"), Variable("logical", x@value@dims))

• Unary plus/minus:

  • R/r2f.R:660–R/r2f.R:669, R/r2f.R:671–R/r2f.R:680
  • Replace ..., x@value) with ..., Variable(x@value@mode, x@value@dims)).

• abs (both places):

  • R/r2f.R:542–R/r2f.R:546: replace arg@value with Variable(arg@value@mode, arg@value@dims).
  • R/r2f.R:586–R/r2f.R:593: for complex input, return Variable("double", arg@value@dims); otherwise same as above.

• Other unary elementals (sin, cos, tan, asin, acos, atan, sqrt, exp, log, floor, ceiling):

  • R/r2f.R:552–R/r2f.R:572: replace arg@value with Variable(arg@value@mode, arg@value@dims).

Optional helper

• To keep code concise, introduce a small helper:

  new_unary_value <- function(v, mode = v@mode) Variable(mode, v@dims)

  Then use new_unary_value(arg@value) or new_unary_value(arg@value, "double") where needed.

Evidence tests

• All current tests pass (testthat::test_local()), but the failing case above isn’t covered. A targeted test would catch regressions:
  • “logical arg; local y <- !x; then ifelse(y, 1L, 0L) compiles and runs.”

Key code references

• Symbol handling that casts external logicals: R/r2f.R:90
• Unary handlers forwarding arg@value:
  • R/r2f.R:660, R/r2f.R:671, R/r2f.R:743, R/r2f.R:542, R/r2f.R:586, R/r2f.R:552

TL;DR

• The pattern of returning Fortran(..., arg@value) in unary handlers makes locals inherit input-argument identity and triggers external-only casts, yielding invalid comparisons in generated Fortran. Constructing a fresh Variable(mode, dims) for unary results fixes the issue.