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

Vet regression case from update #68

Open
benw10-1 opened this issue Oct 17, 2024 · 4 comments · May be fixed by #70
Open

Vet regression case from update #68

benw10-1 opened this issue Oct 17, 2024 · 4 comments · May be fixed by #70

Comments

@benw10-1
Copy link

benw10-1 commented Oct 17, 2024

Seeing stuff that didn't fail on ed6a65f now failing on current commit.

Example:
integrations/testbodyclose/test.go

package sandbox

import (
	"io"
	"net/http"
)

func Foo(req *http.Request) error {
	res, err := http.DefaultClient.Do(req)
	if err != nil {
		return err
	}
	defer res.Body.Close()

	if res.StatusCode == 200 {
		body, err := io.ReadAll(res.Body)
		if err != nil {
			return err
		}
		_ = body

		return nil
	}

	return nil
}

Passes on ed6a65f, but fails with:

integrations/testbodyclose/test.go:10:35: response body must be closed

on master commit.

Can provide more examples similar to this if necessary, but would need to dig them up so didn't include in initial.

@ldez
Copy link
Contributor

ldez commented Oct 17, 2024

Other variants:

if
package sandbox

import (
	"io"
	"net/http"
)

func Foo(req *http.Request) error {
	res, err := http.DefaultClient.Do(req)
	if err != nil {
		return err
	}

	defer res.Body.Close()

	if res.StatusCode == 200 {
		return nil
	}

	io.ReadAll(res.Body)

	return nil
}
for
package sandbox

import (
	"io"
	"net/http"
)

func Foo(req *http.Request) error {
	res, err := http.DefaultClient.Do(req)
	if err != nil {
		return err
	}

	defer res.Body.Close()

	for range res.StatusCode {
		return nil
	}

	io.ReadAll(res.Body)

	return nil
}
switch
package sandbox

import (
	"io"
	"net/http"
)

func Foo(req *http.Request) error {
	res, err := http.DefaultClient.Do(req)
	if err != nil {
		return err
	}

	defer res.Body.Close()

	switch res.StatusCode {
	case http.StatusOK:
		return nil
	}

	io.ReadAll(res.Body)

	return nil
}

@ldez
Copy link
Contributor

ldez commented Oct 17, 2024

The problem is related to the way the graph is analyzed: the algorithm is always wrong if there is at least one if/for/switch/etc. and a defer at the top level.

@benw10-1
Copy link
Author

benw10-1 commented Oct 17, 2024

Seems like it needs to be updated such that it doesn't fail if the parent branch explicitly calls the Close method instead of checking block by block without considering these cases.

I tested the below cases as well and got fails:

Defer Close with "named" block

integrations/testbodyclose/test.go

package sandbox

import (
	"net/http"
)

func Foo(req *http.Request) error {
	res, err := http.DefaultClient.Do(req)
	if err != nil {
		// even though in practice this wouldn't be done, was checking to see if it was this block causing issue
		res.Body.Close()
		return err
	}
	defer res.Body.Close()

	{
		_ = res.Body
	}

	return nil
}

Fails with:

integrations/testbodyclose/test.go:8:35: response body must be closed
Explicit Close with "named" block

integrations/testbodyclose/test.go

package sandbox

import (
	"net/http"
)

func Foo(req *http.Request) error {
	res, err := http.DefaultClient.Do(req)
	if err != nil {
		// even though in practice this wouldn't be done, was checking to see if it was this block causing issue
		res.Body.Close()
		return err
	}

	{
		_ = res.Body
	}

	err = res.Body.Close()
	if err != nil {
		return err
	}

	return nil
}

Fails with:

integrations/testbodyclose/test.go:8:35: response body must be closed

@ldez

This comment was marked as outdated.

@ldez ldez linked a pull request Nov 26, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants