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

Flaky results: Same rule produces different results (Root cause: regexp not getting parsed) #57

Closed
krishna-birla opened this issue Jul 12, 2020 · 18 comments · Fixed by #63
Assignees
Labels
bug Something isn't working

Comments

@krishna-birla
Copy link

My rules:

func _(m fluent.Matcher) {
	m.Match(`$s != $t`).Where(m["s"].Const && !m["t"].Const).Suggest(`$t != $s`)
	m.Match(`$s == $t`).Where(m["s"].Const && !m["t"].Const).Suggest(`$t == $s`)
	m.Match(`$s >= $t`).Where(m["s"].Const && !m["t"].Const).Suggest(`$t <= $s`)
	m.Match(`$s <= $t`).Where(m["s"].Const && !m["t"].Const).Suggest(`$t >= $s`)
	m.Match(`$s > $t`).Where(m["s"].Const && !m["t"].Const).Suggest(`$t < $s`)
	m.Match(`$s < $t`).Where(m["s"].Const && !m["t"].Const).Suggest(`$t > $s`)
	m.Match(`nil == $s`).Where(!m["s"].Const).Suggest(`$s == nil`)
	m.Match(`nil != $s`).Where(!m["s"].Const).Suggest(`$s != nil`)
	m.Match(`log.Infof($s)`).Where(m["s"].Const && m["s"].Type.Is(`string`)).Suggest(`log.Info($s)`)
	m.Match(`log.Warnf($s)`).Where(m["s"].Const && m["s"].Type.Is(`string`)).Suggest(`log.Warn($s)`)
	m.Match(`log.Errorf($s)`).Where(m["s"].Const && m["s"].Type.Is(`string`)).Suggest(`log.Error($s)`)
}

Results:
Screenshot 2020-07-12 at 11 45 05 AM

@quasilyte quasilyte added the bug Something isn't working label Jul 12, 2020
@quasilyte
Copy link
Owner

quasilyte commented Jul 12, 2020

I'm trying to find a minimal repro. It's not easily reproduced in isolation.

Could you explain what do you mean by "regexp not getting parsed"?

Flaky usually means that you get different results from time to time. Does this issue happen only once in a while?

Based on the screenshot provided, I'm seeing that $s is inserted instead of the matched string.

@krishna-birla
Copy link
Author

Could you explain what do you mean by "regexp not getting parsed"?

Basically, $s is not getting replaced back with original string.

Flaky usually means that you get different results from time to time. Does this issue happen only once in a while?

Yes, sometimes it gets parsed properly but then other lines show error.

Main question is, why is $s coming up?

@quasilyte
Copy link
Owner

In order to understand why it happens, I need to reproduce it. I'll try finding the minimal repro.

@krishna-birla
Copy link
Author

Thanks! Please tell me if any more info/help required from me regarding this.

@quasilyte
Copy link
Owner

  1. Is there anything special about the files your running rules on?
    For example: can they be compiled by the Go command or they miss some dependencies?

  2. Can you still reproduce this issue if there is only 1 pattern? e.g.:

m.Match(`log.Infof($s)`).Where(m["s"].Const && m["s"].Type.Is(`string`)).Suggest(`log.Info($s)`)

@krishna-birla
Copy link
Author

  1. Nothing special about my go projects, it is a go module which works perfectly fine with go vet/fmt/build/install.

  2. Tried this rules.go:

package gorules

import "github.com/quasilyte/go-ruleguard/dsl/fluent"

func _(m fluent.Matcher) {
	//m.Match(`$s != $t`).Where(m["s"].Const && !m["t"].Const).Suggest(`$t != $s`)
	//m.Match(`$s == $t`).Where(m["s"].Const && !m["t"].Const).Suggest(`$t == $s`)
	//m.Match(`$s >= $t`).Where(m["s"].Const && !m["t"].Const).Suggest(`$t <= $s`)
	//m.Match(`$s <= $t`).Where(m["s"].Const && !m["t"].Const).Suggest(`$t >= $s`)
	//m.Match(`$s > $t`).Where(m["s"].Const && !m["t"].Const).Suggest(`$t < $s`)
	//m.Match(`$s < $t`).Where(m["s"].Const && !m["t"].Const).Suggest(`$t > $s`)
	//m.Match(`nil == $s`).Where(!m["s"].Const).Suggest(`$s == nil`)
	//m.Match(`nil != $s`).Where(!m["s"].Const).Suggest(`$s != nil`)
	m.Match(`log.Infof($s)`).Where(m["s"].Const && m["s"].Type.Is(`string`)).Suggest(`log.Info($s)`)
	//m.Match(`log.Warnf($s)`).Where(m["s"].Const && m["s"].Type.Is(`string`)).Suggest(`log.Warn($s)`)
	//m.Match(`log.Errorf($s)`).Where(m["s"].Const && m["s"].Type.Is(`string`)).Suggest(`log.Error($s)`)
}

Results still same:
Screenshot 2020-07-13 at 3 52 01 PM

@quasilyte
Copy link
Owner

Thank you! This is helpful indeed.
It will ease the issue pinpointing significantly.

@krishna-birla
Copy link
Author

Seeing this issue in other cases also. Example,

My rules:

func _(m fluent.Matcher) {
	m.Match(`$s != $t`).Where(m["s"].Const && !m["t"].Const).Suggest(`$t != $s`)
	m.Match(`$s == $t`).Where(m["s"].Const && !m["t"].Const).Suggest(`$t == $s`)
	m.Match(`$s >= $t`).Where(m["s"].Const && !m["t"].Const).Suggest(`$t <= $s`)
	m.Match(`$s <= $t`).Where(m["s"].Const && !m["t"].Const).Suggest(`$t >= $s`)
	m.Match(`$s > $t`).Where(m["s"].Const && !m["t"].Const).Suggest(`$t < $s`)
	m.Match(`$s < $t`).Where(m["s"].Const && !m["t"].Const).Suggest(`$t > $s`)
	m.Match(`nil == $s`).Where(!m["s"].Const).Suggest(`$s == nil`)
	m.Match(`nil != $s`).Where(!m["s"].Const).Suggest(`$s != nil`)
	//m.Match(`log.Infof($s)`).Where(m["s"].Const && m["s"].Type.Is(`string`)).Suggest(`log.Info($s)`)
	//m.Match(`log.Warnf($s)`).Where(m["s"].Const && m["s"].Type.Is(`string`)).Suggest(`log.Warn($s)`)
	//m.Match(`log.Errorf($s)`).Where(m["s"].Const && m["s"].Type.Is(`string`)).Suggest(`log.Error($s)`)
}

Results:
Screenshot 2020-07-15 at 4 38 58 PM

@krishna-birla
Copy link
Author

Please feel free to modify the issue title if you think the root cause is something else.

Although, I wonder why someone else did not report this error. This seems to be a big blocker. Is it because I am using [email protected] ?

@krishna-birla
Copy link
Author

Another thing is error happens on the exact same lines, so that is deterministic. Even if I edit the file, it happens in that line of code again.

@krishna-birla
Copy link
Author

Yet another speculation is it happens with same string (in this case). For example, in current run I faced error in two files:

First file with: clusterContext.PostInstallData.CoreDNSUpdateFunction
Second file with: len(recursiveCreateMessage.RecursiveCreateResults)

This specific string got replaced by $s in whole file. Hope this helps.

@quasilyte
Copy link
Owner

Sorry for not fixing this for quite a long time.

I'm having busy weeks on my work. :(
Don't be afraid though. It's on my radar.

Thank you for your collaboration and patience. ❤️

@krishna-birla
Copy link
Author

No issues. <3

@quasilyte quasilyte self-assigned this Jul 19, 2020
@quasilyte
Copy link
Owner

I see what's the problem there.

Before -fix was introduced, all warnings were printed to the stdout and in order to keep things fit a single line nicely, there was a limit of what will be interpolated (basically, everything longer than 40 chars was not replaced).

I believe it still makes sense to keep messages somewhat limited (you can match a huge statement with a pattern), but do a full replacement for the replacement suggestions that are used in -fix mode.

I'm planning this fix:

  1. For the command-line output, increase the width limit to 60 (instead of 40).
  2. When applying -fix suggestions, ignore the width limit.

quasilyte added a commit that referenced this issue Jul 19, 2020
It does make sense to truncate output when printing the
suggestions to the stdout. It's bad if we try to apply
the truncated suggestions to a file though, it leads
to a broken syntax with meta variables inserted instead
of the actual replacements.

1. For the command-line output, increase the width limit to 60 (instead of 40).
2. When applying -fix suggestions, ignore the width limit.

Fixes #57

Signed-off-by: Iskander Sharipov <[email protected]>
quasilyte added a commit that referenced this issue Jul 19, 2020
It does make sense to truncate output when printing the
suggestions to the stdout. It's bad if we try to apply
the truncated suggestions to a file though, it leads
to a broken syntax with meta variables inserted instead
of the actual replacements.

1. For the command-line output, increase the width limit to 60 (instead of 40).
2. When applying -fix suggestions, ignore the width limit.

Fixes #57

Signed-off-by: Iskander Sharipov <[email protected]>
@krishna-birla
Copy link
Author

Wow, great catch! Thanks a lot!! @quasilyte

@krishna-birla
Copy link
Author

@quasilyte Will this fix be available in v0.1.2, or just in master?

@quasilyte
Copy link
Owner

I released v0.1.3 which includes this fix.

@krishna-birla
Copy link
Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants