odin-lang / Odin

Odin Programming Language
https://odin-lang.org
BSD 3-Clause "New" or "Revised" License
7k stars 621 forks source link

Inconsistent typing with #config (string vs integer) #4481

Open lmbarros opened 1 week ago

lmbarros commented 1 week ago

Context

# odin report
Where to find more information and get into contact when you encounter a bug:

    Website: https://odin-lang.org
    GitHub:  https://github.com/odin-lang/Odin/issues

Useful information to add to a bug report:

    Odin:    dev-2024-11-nightly
    OS:      Manjaro Linux, Linux 6.11.2-4-MANJARO
    CPU:     Intel(R) Core(TM) i7-10750H CPU @ 2.60GHz
    RAM:     15900 MiB
    Backend: LLVM 18.1.6

Expected Behavior

I expect the type of a #config(...) expression to be consistent.

Current Behavior

The type changes depending on the value passed in the command-line.

Failure Information (for bugs)

Steps to Reproduce

Take this little program:

package main

import "core:fmt"

main :: proc() {
    fmt.printfln("Commit: %v", #config(COMMIT, "default"))
}

I would expect the type of #config(...) to be always a string (because the default value is a string). However, depending on the value defined, it is parsed as a different type, and this may cause problems:

# This is fine:
# odin run . -define:COMMIT=51987d5cec057b3fbef85c405e8e7a16154fb692
Commit: 51987d5cec057b3fbef85c405e8e7a16154fb692

# This is not:
# odin run . -define:COMMIT=5e987d5cec057b3fbef85c405e8e7a16154fb692
main.odin(6:29) Error: Cannot convert numeric value '1000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000' from '#config(COMMIT, "default")' to 'int' from 'untyped integer' 
    fmt.printfln("Commit: %v", #config(COMMIT, "default")) 
                               ^~~~~~~~~~~~~~~~~~~~~~~~~^ 
    The maximum value that can be represented by 'int' is '9223372036854775807' 

Looks like the hash is being interpreted as a number (5e987, I guess).

Kelimion commented 1 week ago

Try quoting it.

odin run . -vet -strict-style -warnings-as-errors -define:COMMIT="51987d5cec057b3fbef85c405e8e7a16154fb692"

lmbarros commented 1 week ago

Unfortunately, it doesn't seem to work, @Kelimion :

# odin run . -define:COMMIT="5e987d5cec057b3fbef85c405e8e7a16154fb692"
/home/lmb/Lixo/odin-bug/main.odin(6:29) Error: Cannot convert numeric value '1000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000' from '#config(COMMIT, "default")' to 'int' from 'untyped integer' 
    fmt.printfln("Commit: %v", #config(COMMIT, "default")) 
                               ^~~~~~~~~~~~~~~~~~~~~~~~~^ 
    The maximum value that can be represented by 'int' is '9223372036854775807' 

But, honest question, even if this worked, wouldn't this be a (badly) surprising behavior to have an expression like #config(COMMIT, "default") to end up having a type any other than string? I may be missing some detail, but it looks like having the type determined by the default value would be the least surprising behavior.

lmbarros commented 1 week ago

BTW, given the example command-line you provided, I am not sure you understood the issue. Your example works fine even without quotes. The problem seems to be that strings like "5e987..." get interpreted as a huge number because it is being somehow interpreted as if it was a floating point value with a huge exponent.

I also guess this may be a regression, because I had this working on my Makefile for months without issues, and after upgrading to the latest compiler I got this two times already, with two different commits matching this "large exponent" pattern.

lmbarros commented 1 week ago

And just in case other stumble upon the same issue, the workaround I am using is this:

  1. Prepend an underscore to the definition. For example, in my Makefile I am using something like this: ODIN_DEFINES = -define:BUILD_COMMIT="_${BUILD_COMMIT}"
  2. In my Odin code, use #config(BUILD_COMMIT, "_?")[1:] to "discard" the underscore.
Kelimion commented 1 week ago

-define uses the same ExactValue machinery as constants in .odin files do on purpose, precisely for consistency and because it's useful to have string, integer and boolean constants, rather than string constants only.

<snip outdated bits, could indeed replicate it>

lmbarros commented 1 week ago

You are using "51987d5cec057b3fbef85c405e8e7a16154fb692" in your commands. Try "5e987d5cec057b3fbef85c405e8e7a16154fb692". Notice the difference in the second character: e instead of 1.

Kelimion commented 1 week ago

Strange. Must've copy/pasted the wrong constant from the first post.

Kelimion commented 1 week ago

But, honest question, even if this worked, wouldn't this be a (badly) surprising behavior to have an expression like #config(COMMIT, "default") to end up having a type any other than string? I may be missing some detail, but it looks like having the type determined by the default value would be the least surprising behavior.

I agree that's surprising. Here's what happens. The defines are parsed and handled before parsing and type checking of .odin files is kicked off. There's no type information from the later stages to inform -define to be parsed as a string after the fact. Postponing parsing of defines until the type check stage might be possible, but that feels like an invasive change.

What @gingerBill might want to think about is adding type mismatch error when you have #config(FOO, "a string"), but the constant has been parsed as an integer (or bool). Because as it stands, -define:FOO=12 now prints 12 (int) even though "default" is clearly a string.

fmt.printfln("Commit: %v (%T)", #config(FOO, "default"), #config(FOO, "default"))
Kelimion commented 1 week ago

It replicates on Ubuntu and Windows with the 5e, even if the constant is quoted. Very surprising to me, because I explicitly added the ability to quote string defines if they might otherwise be interpreted as an int or bool.

Kelimion commented 1 week ago

As an aside, as a workaround I tried it with single quotes:

W:\Odin\bug>odin run . -define:FOO='5e987d5cec057b3fbef85c405e8e7a16154fb692'
FOO: 5e987d5cec057b3fbef85c405e8e7a16154fb692 (string)

But it still gives the same error of a too large integer on Ubuntu.

lmbarros commented 1 week ago

Thanks! I've been enjoying Odin a lot, BTW!

Not sure if related, but parsing of integers with exponents seems a bit fishy. This program

main :: proc() {
    i: int = 2e1
    j: int = 2.2e1
    fmt.printfln("%v, %v", i, j)
}

gives me this result:

10, 22

(To be honest, I didn't even know we could use the exponent syntax with integers.)

Kelimion commented 1 week ago

2e1 should be 20, not 10. Worth looking into. Could you split that off into its own issue? It's unrelated to how -define detects and parses the different value types.