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

Complex structs with custom to_toml lead to compile time errors #19300

Closed
qtc-de opened this issue Sep 7, 2023 · 4 comments
Closed

Complex structs with custom to_toml lead to compile time errors #19300

qtc-de opened this issue Sep 7, 2023 · 4 comments
Labels
Bug This tag is applied to issues which reports bugs.

Comments

@qtc-de
Copy link
Contributor

qtc-de commented Sep 7, 2023

Describe the bug

A few weeks ago, a generic encode method was added to the toml module (1bed0b5). While this is pretty useful, the implementation is flawed and it is no longer possible to encode complex types that have a custom to_toml method. This is caused by the default encoding method being always part of the encode method. This method expects types to be encodeable and only specific types are allowed (need to be castable to toml.Any at some point of time). However, from my point of view, each type should be encodable as long as it defines a to_toml method.

Reproduction Steps

The following code lists a minimal example:

import toml

struct Example {
	array []Problem
}

struct Problem {
}

pub fn (example Example) to_toml() string
{
	return '[This is Valid]'
}

fn main()
{
	example := Example{}
	toml.encode(example)
}

Expected Behavior

The above example should compile, as the toml.encode method should return [This is Valid], which is valid toml produced by the custom to_toml method.

Current Behavior

The following compile time error is thrown:

v/vlib/toml/toml.v:102:12: error: cannot cast `Problem` to `toml.Any`
  100 |             mut arr := []Any{}
  101 |             for v in value {
  102 |                 arr << Any(v)
      |                        ~~~~~~
  103 |             }
  104 |             mp[field.name] = arr

Possible Solution

The problem is caused by the following function

pub fn encode[T](typ T) string {

pub fn encode[T](typ T) string {
	$for method in T.methods {
		$if method.name == 'to_toml' {
			return typ.$method()
		}
	}

	mp := encode_struct[T](typ)
	return mp.to_toml()
}

If a to_toml method exists, the compile time statements in the beginning create the proper return value. However, the rest of the function seems to be still evaluated by the type checker and causes the error. The most obvious solution would be to prevent compilation of the latter part, if a to_toml method has been found.

I thought about possible implementations, but it seems not that easy. What I could think of is an $if implements operator, which could be used to replace the for loop with an if else condition (after creating an Interface like CustomEncoder that requires implementors to define a to_toml method). Another possible solution would be something like a $break statement, that drops all following definitions within a function once it is hit. No idea if this makes sense or not 🤷

Additional Information/Context

No response

V version

V 0.4.1 3042857.f18086c

Environment details (OS name and version, etc.)

Not relevant

@qtc-de qtc-de added the Bug This tag is applied to issues which reports bugs. label Sep 7, 2023
@ttytm
Copy link
Member

ttytm commented Sep 13, 2023

That's true. We really need to make sure it short circuits after the return without doing the rest of the comptime evaluation.

Doing some quick tries I can't force it to early return without the comptime evaluation.
Improving the generic encoding function to not run into the type error can fix this as well, but stoping further evaluation after a return would be 💯

@spytheman
Copy link
Member

@qtc-de can you please confirm if your problem is solved after v up (latest v)?

@spytheman
Copy link
Member

I am closing this, since I think #19338 solved it.

@qtc-de
Copy link
Contributor Author

qtc-de commented Sep 21, 2023

Sorry for the late response. #19338 indeed solved the edge case given above. However, I only chose this edge case to provide a basic example. My actual problem is related to the map type not being handled correctly, which still causes compile time errors in the latest version.

Based on #19338 I just submitted #19408 which catches my edge case and hopefully most others 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug This tag is applied to issues which reports bugs.
Projects
None yet
Development

No branches or pull requests

3 participants