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

checker: add warn about non-option function #18560

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

felipensp
Copy link
Member

@felipensp felipensp commented Jun 25, 2023

This PR is about the ROADMAP item "Handle function pointers safely, remove if function == 0 {".

type MapFunc = fn () 

struct Test {
	a fn (int) // warn
	b ?fn () // ok
	c MapFunc // warn
	d ?MapFunc // ok
}

fn main() {
	t := Test{b: fn () {
		println('cool')
	}}
	z := t.b?
	z()
	dump(t)
}
$ v run bug.v
bug.v:2:16: warning: you should declare it as Option instead
    1 | 
    2 | type MapFunc = fn () 
      |                ~~~~~~
    3 | 
    4 | struct Test {
bug.v:5:4: warning: you should declare it as Option instead (?fn ...)
    3 | 
    4 | struct Test {
    5 |     a fn (int) // warn
      |       ~~~~~~~~
    6 |     b ?fn () // ok
    7 |     c MapFunc // warn
bug.v:7:4: warning: this is not recommended, you should use Option function instead
    5 |     a fn (int) // warn
    6 |     b ?fn () // ok
    7 |     c MapFunc // warn
      |       ~~~~~~~
    8 |     d ?MapFunc // ok
    9 | }
cool
[bug.v:17] t: Test{
    a: fn (int)
    b: Option(fn ())
    c: fn ()
    d: Option(none)
}

🤖 Generated by Copilot at 100f441

Warn about direct function types in struct fields. The change modifies vlib/v/checker/struct.v to emit a warning when a struct field is declared as a direct function type without the option flag, as this can cause memory issues. The change is part of a pull request to improve function type safety in V.

🤖 Generated by Copilot at 100f441

  • Add a warning message for direct function types in struct fields (link)

@felipensp
Copy link
Member Author

felipensp commented Jun 25, 2023

@medvednikov Should we make possible to do "t.b?()" too?

@spytheman
Copy link
Member

imho, this should be a notice instead of a warning, at least for now

@ArtemkaKun
Copy link
Contributor

ArtemkaKun commented Jul 3, 2023

This PR also closes #9977 probably. Or not?

@spytheman spytheman added Needs Rebase The code of the PR must be rebased over current master before it can be approved. Breaking Change This PR introduces changes that break backward compatibility. Requires manual review. labels Aug 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change This PR introduces changes that break backward compatibility. Requires manual review. Needs Rebase The code of the PR must be rebased over current master before it can be approved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants