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.

Sign up for GitHub

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

checker,cgen: fix duplicates in generic sumtype #20936

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
+130 −7
Open

checker,cgen: fix duplicates in generic sumtype #20936

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5731fc6
fix
MCausc78 Mar 1, 2024
66f7a19
doc
MCausc78 Mar 1, 2024
5ff4deb
ci fix
MCausc78 Mar 1, 2024
b08058c
link issue instead of job
MCausc78 Mar 1, 2024
ea4c086
apply delta suggestions
MCausc78 Mar 5, 2024
d7950ed
didn't mean to add test.v
MCausc78 Mar 5, 2024
8ab9a27
remove `@[ignore_generic_duplicates]` attribute in `Name5`
MCausc78 Mar 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions vlib/v/ast/types.v
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,8 @@ pub mut:

@[minify]
pub struct SumType {
pub:
attrs []Attr
pub mut:
fields []StructField
found_fields bool
Expand All @@ -262,6 +264,17 @@ pub mut:
parent_type Type
}

// used to check whether there a generic type which is duplicate, returns array with no duplicates
pub fn (st &SumType) get_deduplicated_variants() []Type {
mut variants := []Type{}
for v in st.variants {
if v !in variants {
variants << v
}
}
return variants
}

// <atomic.h> defines special typenames
pub fn (t Type) atomic_typename() string {
idx := t.idx()
Expand Down
16 changes: 16 additions & 0 deletions vlib/v/checker/checker.v
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3082,6 +3082,22 @@ fn (mut c Checker) cast_expr(mut node ast.CastExpr) ast.Type {
ft := c.table.type_to_str(from_type)
tt := c.table.type_to_str(to_type)
c.error('cannot cast `${ft}` to `${tt}`', node.pos)
} else if to_sym_info.variants.len != to_sym_info.get_deduplicated_variants().len {
// not a option type because of autofree bug; see https://github.com/vlang/v/actions/runs/8116684464/job/22187255774?pr=20936
MCausc78 marked this conversation as resolved.
Show resolved Hide resolved
mut msg := ''
print_notice := to_sym_info.attrs.any(!it.has_arg && it.name == 'notice_if_duplicate')
for attr in to_sym_info.attrs {
if attr.name == 'on_duplicate' && attr.has_arg {
msg = attr.arg
}
}
if msg.len > 0 {
if print_notice {
c.note('duplicate type: ${msg}', node.pos)
} else {
c.error('duplicate type: ${msg}', node.pos)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, there is no existing precedent for using an attribute for toggling between an error and a notice 🤔. I am not sure if it is worth the complexity.
Would not either just a notice, or either just an error be simpler?

Copy link
Contributor Author

@MCausc78 MCausc78 Mar 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sometimes this is bad idea to give error. Consider that:
firstapi:

pub struct Row[T] {
pub mut:
	// children components
	components []Component[T]
}

@[on_duplicate: 'T is present in sumtype']
@[notice_if_duplicate]
pub type Component[T] = Row[Button]
	| Button
	| StringSelect
	| TextInput
	| T

secondapi:

struct Default[T] {
	v T
}

pub fn c[T]() Component[T] {
	return Component[T](Default[T]{}.v)
}

user (not dev of firstapi and secondapi packages), not knowing that e.g. TextInput is present in Component type, uses c[TextInput](), and gets an notice, instead of hard error. That also would allow deprecating duplicates in generic sumtypes.
Yet one example:

struct Undefined {}

@[on_duplicate: 'Sentinels are internal']
pub type UndefinedOr[T] = Undefined | T

That would give a error if user will do UndefinedOr[Undefined].

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@medvednikov what do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @spytheman here. This complicates the language and gives too many options. I'd just stick with one option.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed attribute and made error by default

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are still 2 attributes:
ignore_generic_duplicates and on_generic_duplicate.

I do not know how to document/explain that in doc/docs.md.

}
}
} else if mut to_sym.info is ast.Alias && !(final_to_sym.kind == .struct_ && final_to_is_ptr) {
if (!c.check_types(from_type, to_sym.info.parent_type) && !(final_to_sym.is_int()
Expand Down
13 changes: 13 additions & 0 deletions vlib/v/checker/tests/notice_or_error_on_duplicate_using_generic.out
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
vlib/v/checker/tests/notice_or_error_on_duplicate_using_generic.vv:9:7: notice: duplicate type: blah blah blah 11111
7 |
8 | fn main() {
9 | _ := Name1[int](123)
| ~~~~~~~~~~~~~~~
10 | _ := Name2[int](456)
11 | }
vlib/v/checker/tests/notice_or_error_on_duplicate_using_generic.vv:10:7: error: duplicate type: blah blah blah 22222
8 | fn main() {
9 | _ := Name1[int](123)
10 | _ := Name2[int](456)
| ~~~~~~~~~~~~~~~
11 | }
11 changes: 11 additions & 0 deletions vlib/v/checker/tests/notice_or_error_on_duplicate_using_generic.vv
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
@[on_duplicate: 'blah blah blah 11111']
@[notice_if_duplicate]
type Name1[T] = T | int | string

@[on_duplicate: 'blah blah blah 22222']
type Name2[T] = T | int | string

fn main() {
_ := Name1[int](123)
_ := Name2[int](456)
}
15 changes: 9 additions & 6 deletions vlib/v/gen/c/cgen.v
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -925,7 +925,7 @@ pub fn (mut g Gen) write_typeof_functions() {
g.writeln('${static_prefix}char * v_typeof_sumtype_${sym.cname}(int sidx) { /* ${sym.name} */ ')
if g.pref.build_mode == .build_module {
g.writeln('\t\tif( sidx == _v_type_idx_${sym.cname}() ) return "${util.strip_main_name(sym.name)}";')
for v in sum_info.variants {
for v in sum_info.get_deduplicated_variants() {
subtype := g.table.sym(v)
g.writeln('\tif( sidx == _v_type_idx_${subtype.cname}() ) return "${util.strip_main_name(subtype.name)}";')
}
Expand All @@ -934,7 +934,7 @@ pub fn (mut g Gen) write_typeof_functions() {
tidx := g.table.find_type_idx(sym.name)
g.writeln('\tswitch(sidx) {')
g.writeln('\t\tcase ${tidx}: return "${util.strip_main_name(sym.name)}";')
for v in sum_info.variants {
for v in sum_info.get_deduplicated_variants() {
subtype := g.table.sym(v)
g.writeln('\t\tcase ${v.idx()}: return "${util.strip_main_name(subtype.name)}";')
}
Expand All @@ -946,7 +946,7 @@ pub fn (mut g Gen) write_typeof_functions() {
g.writeln('${static_prefix}int v_typeof_sumtype_idx_${sym.cname}(int sidx) { /* ${sym.name} */ ')
if g.pref.build_mode == .build_module {
g.writeln('\t\tif( sidx == _v_type_idx_${sym.cname}() ) return ${int(ityp)};')
for v in sum_info.variants {
for v in sum_info.get_deduplicated_variants() {
subtype := g.table.sym(v)
g.writeln('\tif( sidx == _v_type_idx_${subtype.cname}() ) return ${int(v)};')
}
Expand All @@ -955,7 +955,7 @@ pub fn (mut g Gen) write_typeof_functions() {
tidx := g.table.find_type_idx(sym.name)
g.writeln('\tswitch(sidx) {')
g.writeln('\t\tcase ${tidx}: return ${int(ityp)};')
for v in sum_info.variants {
for v in sum_info.get_deduplicated_variants() {
g.writeln('\t\tcase ${v.idx()}: return ${int(v)};')
}
g.writeln('\t\tdefault: return ${int(ityp)};')
Expand Down Expand Up @@ -6399,13 +6399,16 @@ fn (mut g Gen) write_types(symbols []&ast.TypeSymbol) {
struct_names[name] = true
g.typedefs.writeln('typedef struct ${name} ${name};')
g.type_definitions.writeln('')

g.type_definitions.writeln('// Union sum type ${name} = ')
for variant in sym.info.variants {
variants := sym.info.get_deduplicated_variants()
for variant in variants {
g.type_definitions.writeln('// | ${variant:4d} = ${g.typ(variant.idx()):-20s}')
}
g.type_definitions.writeln('struct ${name} {')
g.type_definitions.writeln('\tunion {')
for variant in sym.info.variants {

for variant in variants {
variant_sym := g.table.sym(variant)
mut var := variant.ref()
if variant_sym.info is ast.FnType {
Expand Down
1 change: 1 addition & 0 deletions vlib/v/parser/parser.v
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4373,6 +4373,7 @@ fn (mut p Parser) type_decl() ast.TypeDecl {
cname: util.no_dots(prepend_mod_name)
mod: p.mod
info: ast.SumType{
attrs: p.attrs
variants: variant_types
is_generic: generic_types.len > 0
generic_types: generic_types
Expand Down
14 changes: 14 additions & 0 deletions vlib/v/tests/ignore_duplicate_using_generic_test.v
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
type Name[T] = T | int | string

fn f(n Name[string]) {
assert n as string == 'lolol'
}

fn g(n Name[int]) {
assert n as int == 123
}

fn test_main() {
f(Name[string]('lolol'))
g(Name[int](123))
}