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

Conversion is used on a sum type even if it is unnecessary #7854

Closed
radeusgd opened this issue Sep 20, 2023 · 2 comments · Fixed by #7952
Closed

Conversion is used on a sum type even if it is unnecessary #7854

radeusgd opened this issue Sep 20, 2023 · 2 comments · Fixed by #7952
Assignees
Labels

Comments

@radeusgd
Copy link
Member

See the following example:

from Standard.Base import all

type My_A
    Value x

My_A.from (that:Text) = My_A.Value that

f1 (m : Text | My_A) = m.to_text
f2 (m : My_A | Text) = m.to_text
f3 (m : My_A | Text | Integer) = m.to_text

main =
    IO.println (f1 "TEST")
    IO.println (f2 "TEST")
    IO.println (f3 "TEST")
    IO.println (f3 44)

It currently yields:

TEST
(My_A.Value 'TEST')
(My_A.Value 'TEST')
44

It means that the first type of a sum type is treated specially. In f2, even though my value is already Text - so it already fits the expected type, it gets converted, because a conversion to the first type is found. We can see that when no conversion is found (f3 44) it is passed unchanged okay.

I don't think this is right. If the type already fits the requested sum type - why trigger a conversion?

@radeusgd
Copy link
Member Author

radeusgd commented Sep 20, 2023

This will bite us especially once we start adding Text.from conversions to stuff.

I have a valid usecase, where I want to accept either a Formatter or a raw Text. The formatter is also convertible to Text for other purposes. If I add a conversion from formatter to text (which is the plan, AFAIK), then it will form a cycle and I will never be able to get either Formatter | Text - because due to the cycle always one will be preferred.

See my motivating example - what happens if we add a conversion from Date_Time_Formatter 'back' to text - closing the cycle:

from Standard.Base import all

format_a_date (d : Date) (f : Date_Time_Formatter) = 
    d.format f

format_a_thing thing (f : Text | Date_Time_Formatter) = 
    if f.is_nothing || f == "" then "Default format: "+thing.to_text else
        IO.println "Formatting thing "+thing.to_text+" with "+f.pretty+" ("+(Meta.type_of f).to_text+")"
        thing.format f

# Once I add a conversion to Text, I get a loop and `format_a_thing` will _always_ get the value as the first one in the sum type - but I need to distinguish them
Text.from (that:Date_Time_Formatter) = that.to_text

main =
    IO.println (format_a_date Date.today "dd/MM/yyyy")
    IO.println (format_a_thing Date.today "dd/MM/yyyy")
    f = Date_Time_Formatter.from "dddd, dd MMMM yyyy" Locale.poland
    IO.println (format_a_date Date.today f)
    IO.println (format_a_thing Date.today f)
    IO.println (format_a_thing 42 "#.##")

yielding

20/09/2023
Formatting thing 2023-09-20 with 'dd/MM/yyyy' (Text)
20/09/2023
środa, 20 września 2023
Formatting thing 2023-09-20 with 'Date_Time_Formatter.from_simple_pattern \'dddd, dd MMMM yyyy\'' (Text)
(Error: (Date_Time_Format_Parse_Error.Error 'The pattern e is not recognized as any known pattern for the Simple format.'))
Formatting thing 42 with '#.##' (Text)
42

As we can see, the call format_a_thing Date.today f mistakenly converts f : Date_Time_Formatter into Text and so we call format with a text pattern 'Date_Time_Formatter.from_simple_pattern \'dddd, dd MMMM yyyy\'' which is not a valid pattern. The conversion should not have taken place.

Note that this example relies on changes I did as part of #7826 which is not merged yet.

@jdunkerley jdunkerley moved this from ❓New to 📤 Backlog in Issues Board Sep 26, 2023
@JaroslavTulach JaroslavTulach moved this from 📤 Backlog to 👁️ Code review in Issues Board Oct 3, 2023
mwu-tow pushed a commit that referenced this issue Oct 4, 2023
Fixes #7854 by checking all elements of a union type for direct match and only then, if direct match isn't available, applying conversions.
@github-project-automation github-project-automation bot moved this from 👁️ Code review to 🟢 Accepted in Issues Board Oct 4, 2023
@enso-bot
Copy link

enso-bot bot commented Oct 5, 2023

Jaroslav Tulach reports a new STANDUP for yesterday (2023-10-04):

Progress: - Avoid unnecessary conversions: #7952

Next Day: Bugfixing, websocket polyfill

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

Successfully merging a pull request may close this issue.

2 participants