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

Only flag regexp calls that use values known at compile time #1

Merged
merged 1 commit into from
Sep 11, 2020

Conversation

nmiyake
Copy link
Contributor

@nmiyake nmiyake commented Aug 26, 2020

Do not flag function calls that have variables or function calls
in their arguments, as such code cannot be refactored to only be
called once.

Do not flag function calls that have variables or function calls
in their arguments, as such code cannot be refactored to only be
called once.
@nmiyake
Copy link
Contributor Author

nmiyake commented Aug 26, 2020

Hi, came across your project and wanted to make a suggestion.

It makes sense for a linter to flag cases where a regular expression is being compiled when the content is known at program compile time, but if a program compiles a regular expression based on variables or function calls (based on user input, file content, etc.), then it probably doesn't make sense to flag those occurrences, since they are valid and can't be compiled in a way that satisfies this check. Although the check provides a comment suppression mechanism, it probably makes more sense to handle this directly.

This PR contains an example of how this modification could be made by doing an extra AST check after the initial "called" check.

Copy link
Owner

@budougumi0617 budougumi0617 left a comment

Choose a reason for hiding this comment

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

I appreciate the great PR!

@@ -17,7 +20,7 @@ const doc = `Below functions should be called at once for performance.
- regexp.CompilePOSIX
- regexp.MustCompilePOSIX

Allow call in init, and main(except for in for loop) functions because each function is called only once.
Allow call in init and main functions (unless call is in a for loop) because these functions are only called once.
Copy link
Owner

Choose a reason for hiding this comment

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

👍


regexp.MustCompile(constVal) // want `regexp.MustCompile must be called only once at initialize`
regexp.MustCompile(constVal + constVal) // want `regexp.MustCompile must be called only once at initialize`
}
Copy link
Owner

Choose a reason for hiding this comment

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

excellent test cases!

@@ -73,6 +111,30 @@ func run(pass *analysis.Pass) (interface{}, error) {
return nil, nil
}

// variablesOrCallInCallExpr returns true if the provided *ast.CallExpr has
// at least one argument and if the first argument contains a reference to a
// variable or a function call.
Copy link
Owner

Choose a reason for hiding this comment

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

These comment are easy for me to understand. Thank you for explaining in comments.

@budougumi0617
Copy link
Owner

and, sorry for review so late...

@budougumi0617 budougumi0617 merged commit 6ce6275 into budougumi0617:master Sep 11, 2020
@budougumi0617
Copy link
Owner

I released new version included this PR!
https://github.com/budougumi0617/regexponce/releases/tag/v0.1.1

@nmiyake nmiyake deleted the doNotFlagVarOrCall branch September 11, 2020 20:27
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 this pull request may close these issues.

None yet

2 participants