Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A Time_Of_Day represented by java.time.LocalTime fails TypesGen.expectEnsoTimeOfDay check #6912

Closed
radeusgd opened this issue May 31, 2023 · 10 comments
Assignees
Labels
Milestone

Comments

@radeusgd
Copy link
Member

A Time_Of_Day should behave the same regardless of its source. But that is not the case.

Run this snippet:

from Standard.Base import all
from Standard.Table import all

main =
    data = 'Times\n12:34:56'
    t = Table.from data (Delimited ',')
    t.print

    date = Date.new 2023 05 31
    
    native_tod = Time_Of_Day.new 12 34 56
    IO.println native_tod
    dt1 = date.to_date_time native_tod
    IO.println dt1

    parsed_tod = t.at "Times" . first
    IO.println parsed_tod
    dt2 = date.to_date_time parsed_tod
    IO.println dt2

Expected behaviour

Both cases should work exactly the same:

   | Times
---+----------
 0 | 12:34:56

12:34:56
2023-05-31T12:34:56+02:00[Europe/Warsaw]
12:34:56
2023-05-31T12:34:56+02:00[Europe/Warsaw]

Actual behaviour

   | Times
---+----------
 0 | 12:34:56

12:34:56
2023-05-31T12:34:56+02:00[Europe/Warsaw]
12:34:56
Execution finished with an error: Type error: expected `arg1` to be Time_Of_Day, but got Time_Of_Day.
        at <enso> Date.to_date_time(C:\NBO\enso\built-distribution\enso-engine-0.0.0-dev-windows-amd64\enso-0.0.0-dev\lib\Standard\Base\0.0.0-dev\src\Data\Time\Date.enso:426:79-115)
        at <enso> date-polyglot.main(date-polyglot.enso:18:11-38)

The error is particularly friendly, since the expected and actual types are the same 😅

@radeusgd radeusgd changed the title A parsed Time_Of_Day looks fails a typecheck A parsed Time_Of_Day fails a Time_Of_Day typecheck May 31, 2023
@radeusgd
Copy link
Member Author

Looks like this may likely be some polyglot issue since the 'parsed' date comes from a column so it goes through a Java boundary.

Indeed, nothing to do with parsing:

from Standard.Base import all
from Standard.Table import all

main =
    date = Date.new 2023 05 31

    native_tod = Time_Of_Day.new 12 34 56    
    IO.println native_tod
    dt1 = date.to_date_time native_tod
    IO.println dt1

    table = Table.new [["Times", [native_tod]]]
    table.info.print
    tabled_tod = table.at "Times" . first
    IO.println tabled_tod
    dt2 = date.to_date_time tabled_tod
    IO.println dt2

yields

12:34:56
2023-05-31T12:34:56+02:00[Europe/Warsaw]
   | Column | Items Count | Value Type
---+--------+-------------+------------
 0 | Times  | 1           | Time

12:34:56
Execution finished with an error: Type error: expected `arg1` to be Time_Of_Day, but got Time_Of_Day.
        at <enso> Date.to_date_time(C:\NBO\enso\built-distribution\enso-engine-0.0.0-dev-windows-amd64\enso-0.0.0-dev\lib\Standard\Base\0.0.0-dev\src\Data\Time\Date.enso:426:79-115)
        at <enso> date-polyglot.main(date-polyglot.enso:16:11-38)

@radeusgd radeusgd changed the title A parsed Time_Of_Day fails a Time_Of_Day typecheck A Time_Of_Day coming from a Table fails a Time_Of_Day typecheck May 31, 2023
@radeusgd
Copy link
Member Author

Both values happily answer True to is_a Time_Of_Day.

from Standard.Base import all
from Standard.Table import all

make_a_dt (tod : Time_Of_Day) =
    date = Date.new 2023 05 31
    IO.println tod.to_text+" is a Time_Of_Day? "+(tod.is_a Time_Of_Day).to_text
    date.to_date_time tod

main =
    native_tod = Time_Of_Day.new 12 34 56    
    IO.println native_tod
    dt1 = make_a_dt native_tod
    IO.println dt1

    table = Table.new [["Times", [native_tod]]]
    table.info.print
    tabled_tod = table.at "Times" . first
    IO.println tabled_tod
    dt2 = make_a_dt tabled_tod
    IO.println dt2
12:34:56
12:34:56 is a Time_Of_Day? True
2023-05-31T12:34:56+02:00[Europe/Warsaw]
   | Column | Items Count | Value Type
---+--------+-------------+------------
 0 | Times  | 1           | Time

12:34:56
12:34:56 is a Time_Of_Day? True
Execution finished with an error: Type error: expected `arg1` to be Time_Of_Day, but got Time_Of_Day.
        at <enso> Date.to_date_time(C:\NBO\enso\built-distribution\enso-engine-0.0.0-dev-windows-amd64\enso-0.0.0-dev\lib\Standard\Base\0.0.0-dev\src\Data\Time\Date.enso:426:79-115)
        at <enso> date-polyglot.make_a_dt(date-polyglot.enso:7:5-25)
        at <enso> date-polyglot.main(date-polyglot.enso:19:11-30)

It seems to be some issue with how the builtin dispatch handles polyglot conversions.

Date's to_time_builtin expects a EnsoTimeOfDay and apparently it does not give the Java value coming from the Table a chance to convert to this type and instead fails. This probably affects all our builtins accepting date/time values.

@radeusgd
Copy link
Member Author

Indeed, e.g. to_date_time of Time_Of_Day also suffers from a symmetric issue:
image

@radeusgd
Copy link
Member Author

A repro with no Table usage:

from Standard.Base import all

main =
    tod = Time_Of_Day.new 12 34 56
    native_date = Date.new 2023 05 31

    dt1 = tod.to_date_time native_date
    IO.println dt1

    # next uses +, which uses `Time_Utils.date_adjust` so it 'coerces' the EnsoDate into Java LocalDate
    later = native_date.next
    IO.println later
    dt2 = tod.to_date_time later
    IO.println dt2
2023-05-31T12:34:56+02:00[Europe/Warsaw]
2023-06-01
Execution finished with an error: Type error: expected `arg1` to be Date, but got Date.
        at <enso> Time_Of_Day.to_date_time(C:\NBO\enso\built-distribution\enso-engine-0.0.0-dev-windows-amd64\enso-0.0.0-dev\lib\Standard\Base\0.0.0-dev\src\Data\Time\Time_Of_Day.enso:254:54-88)
        at <enso> date-polyglot.main(date-polyglot.enso:13:11-32)

@JaroslavTulach JaroslavTulach self-assigned this Jun 1, 2023
@JaroslavTulach JaroslavTulach moved this from ❓New to 📤 Backlog in Issues Board Jun 1, 2023
@hubertp hubertp removed the triage label Jun 2, 2023
@JaroslavTulach
Copy link
Member

JaroslavTulach commented Jun 2, 2023

The problem isn't caused by

This problem has been there since ages, I think. In short the TypesGen concept is broken:

TypesGen.expectEnsoTimeOfDay() (engine/runtime/target/scala-2.13/src_managed/main/org/enso/interpreter/runtime/type/TypesGen.java:496)
ToTimeBuiltinDateMethodGen.handleExecute() (engine/runtime/target/scala-2.13/src_managed/main/org/enso/interpreter/node/expression/builtin/date/ToTimeBuiltinDateMethodGen.java:124)
ToTimeBuiltinDateMethodGen$1Inlineable.call() engine/runtime/target/scala-2.13/src_managed/main/org/enso/interpreter/node/expression/builtin/date/ToTimeBuiltinDateMethodGen.java:82)

Image

The to_time_builtin expects EnsoTimeOfDay, but instead it gets java.time.LocalTime and the TypesGen concept is poor enough to make the conversion!

@JaroslavTulach JaroslavTulach changed the title A Time_Of_Day coming from a Table fails a Time_Of_Day typecheck A Time_Of_Day represented by java.time.LocalTime fails TypesGen.expectEnsoTimeOfDay check Jun 2, 2023
@JaroslavTulach JaroslavTulach added this to the Beta Release milestone Jun 2, 2023
mergify bot pushed a commit that referenced this issue Jun 6, 2023
Related to #6912

It essentially solves it by removing any builtins that would take an EnsoDate/EnsoTimeOfDay/EnsoTimeZone and replacing them with Java utils that do the same operation.

This is not a proper solution - the builtin conversion is still invalid for the date/time types - but at this moment we may just no longer use the invalid conversion so it is much less of an issue. We still need to be aware of this if we want to introduce builtins taking date/time in the future.
@jdunkerley jdunkerley moved this from 📤 Backlog to ❓New in Issues Board Jul 4, 2023
@jdunkerley jdunkerley moved this from ❓New to 📤 Backlog in Issues Board Jul 4, 2023
@JaroslavTulach JaroslavTulach moved this from 📤 Backlog to ⚙️ Design in Issues Board Jul 20, 2023
@JaroslavTulach JaroslavTulach moved this from ⚙️ Design to 🔧 Implementation in Issues Board Jul 20, 2023
@JaroslavTulach
Copy link
Member

The given example seems to work. Certainly since #7344

@github-project-automation github-project-automation bot moved this from 🔧 Implementation to 🟢 Accepted in Issues Board Jul 24, 2023
@radeusgd
Copy link
Member Author

I think we should add some unit tests to avoid regressing this in the future.

@radeusgd
Copy link
Member Author

Ah, I realised - of course the problem is not visible, because we've got a workaround in! #6964
Whether the underlying issue is solved or not - I have no idea.
To test it we'd have to create a builtin accepting a date value and check if it works regardless if the value provided is EnsoDate or LocalDate.

@JaroslavTulach
Copy link
Member

JaroslavTulach commented Jul 24, 2023

the problem is not visible, because we've got a workaround in! #6964

You are welcomed to update steps to reproduce and reopen the issue. Update:

we'd have to create a builtin accepting a date value and check if it works regardless if the value

Yes, these conversions are not ideal, but when the code got fixed by removing the builtin, then we can live with that, I think.

@enso-bot
Copy link

enso-bot bot commented Jul 25, 2023

Jaroslav Tulach reports a new STANDUP for yesterday (2023-07-24):

Progress: - #7344

Next Day: Bugfixes & cleanup

Discord
Discord is the easiest way to communicate over voice, video, and text. Chat, hang out, and stay close with your friends and communities.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

No branches or pull requests

3 participants