feat: ability to ignore lint diagnostics in realms

[0xtekgrinder]

Added a wrapper inside osm to walk through a directory recursively
Added a content traversing inside gno lint
Added a way to ignore diagnostics with // nolint

It is responding to #1042

Contributors' checklist...

• Added new tests, or not needed, or not feasible
• Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
• Updated the official documentation or not needed
• No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
• Added references to related issues and PRs
• Provided any useful hints for running manual tests
• Added new benchmarks to generated graphs, if any. More info here.

[deelawn]

Thanks for the contribution @0xtekgrinder!

I left a few comments here for you to address. In general, I think that it is an acceptable and simple solution in order for us to have this functionality quickly. However, I think the more complete solution would be to use the parser package from the standard library to extract the comments.

So if we merge this solution, then we should open a separate issue to follow up with what needs to be fixed. The two things I can think of to include in the issue are:

• Use the go standard library's parser package to parse comments; the current solution would erroneously excludes lines from linting if the text //nolint appears anywhere, even if it is not a comment
• Add support for the //nolint directive to also be included above the declaration/block it is meant to refer to

Let's wait for someone else from the core team to take a look as well.

[0xtekgrinder]

Thanks for the contribution @0xtekgrinder!

I left a few comments here for you to address. In general, I think that it is an acceptable and simple solution in order for us to have this functionality quickly. However, I think the more complete solution would be to use the parser package from the standard library to extract the comments.

So if we merge this solution, then we should open a separate issue to follow up with what needs to be fixed. The two things I can think of to include in the issue are:

• Use the go standard library's parser package to parse comments; the current solution would erroneously excludes lines from linting if the text //nolint appears anywhere, even if it is not a comment
• Add support for the //nolint directive to also be included above the declaration/block it is meant to refer to

Let's wait for someone else from the core team to take a look as well.

I am up to make a cleaner version with the parser as the linter will need to use AST to be effective so makes sense to switch to this directly.

[0xtekgrinder]

@deelawn I am currently implementing what we talked about but I have some problem which is caused by my lack of understanding of the codebase.

As of right now, in the gno ast, there isn't any Comment or CommentGroup Node. I am trying to then create it. I was wondering if I could get some directives on how to do it.

I have pushed what I think might be the beginning to do that but I just can't find where each node implements the function of the Node interface.

[deelawn]

Hey @0xtekgrinder , sorry for the late response; I was out of the office.

There are three things each node does to implement the Node interface:

1. embed the Attributes type
2. define the assertExpr method (search for this in nodes.go)
3. define the String method (see nodes_string.go)

Does that help? Are there other specific questions you have about the implementation of comment nodes?

[0xtekgrinder]

@deelawn Thanks a lot for your answer, it does help indeed.

However I am not sure Comment and CommentGroup should be inside the ExprNode as if we compare to go AST it is apart.
Currently it is a node apart so I still needs to find a way to call it with go2gno function

[thehowl]

Hey, just taking a look at this.

As it currently stands, gno parsing is still done through the Go parser and AST and then converted to Gno (as you are doing here). However, I don't think we should be introducing Comment types and information to the gnolang package. It's meant to be minimal, and to not concern itself with anything happening within the comments really...

I'm not 100% aware of how the linters work, but I suggest that if they are currently using Gno's AST system instead of Go's, to switch it over to Go instead. We are already doing this anyway for the gno doc command (see gnovm/pkg/doc). So it's fine to use that instead, and use the parser with ParseComments.

I understand it can be a bit tricky to sort this out. If you want to discuss on how this can be implemented, please get on Discord's #dev-public channel and feel free to tag me (at-morganbaz) :)

[0xtekgrinder]

Hey, just taking a look at this.

As it currently stands, gno parsing is still done through the Go parser and AST and then converted to Gno (as you are doing here). However, I don't think we should be introducing Comment types and information to the gnolang package. It's meant to be minimal, and to not concern itself with anything happening within the comments really...

I'm not 100% aware of how the linters work, but I suggest that if they are currently using Gno's AST system instead of Go's, to switch it over to Go instead. We are already doing this anyway for the gno doc command (see gnovm/pkg/doc). So it's fine to use that instead, and use the parser with ParseComments.

I understand it can be a bit tricky to sort this out. If you want to discuss on how this can be implemented, please get on Discord's #dev-public channel and feel free to tag me (at-morganbaz) :)

Followed your suggestions and now it should be better. I also added a string for the rule reprensentation to have a similar system such as golangci-lint with a //notlint comment with multiples rules id following it. The code isn't very clean yet. I wants to create another function but first I was wondering is there a reason why the addError is a inline function ?

[thehowl]

So, apologies but I needed a bit of time to take in this and try to come up with how I think we should move forward.

At this point, I dislike this because it requires doing a second round of parsing for every file that has a linting issue. This is slow and inefficient when the parsing already happens for the GnoVM anyway.

I think the way to go about this is actually placing comment parsing in the GnoVM. Sorry to have changed my mind on what I said, but trying to understand how the linter should work is tricky. As you may have noted, it is still pretty rudimentary and we don't many established design features.

However, I'm conflicted. In order to take advantage of parsing comments directly in the GnoVM (this would have to happen in gnolang/go2gno), we'd need a way for the linter to obtain the precise Nodes where the error occurred within the GnoVM.

I suggest you take a look at changing the recover function for Preprocess:

gno/gnovm/pkg/gnolang/preprocess.go

Lines 155 to 157 in 9734695

	defer func() {
	if r := recover(); r != nil {
	// before re-throwing the error, append location information to message.

I suggest you use last, n and ns (NodeStack, this as far as I understand should contain the current "depth" of nodes in the preprocessing tree) to find the last Stmt or Decl being processed. With that, you can put it into PreprocessError, and in conjuction with adding comments as attributes in go2gno, you should be able to avoid double-parsing the source.

I understand this may be more complicated than you're willing to take on. If you need help, please do ping me and I'll try to help you best I can. If the rabbit hole seems too big, that's OK too and we can try to take it on as the core team (sooner or later ™️).

Looking for a second pair of eyes @gfanton @ajnavarro if you see better approaches.

[thehowl]

we're already having rich information when we use addIssue

I suggest you keep this as constants and use stringer to generate a String method for lintCode. See gnovm/Makefile @ _dev.stringer for where to add this.

[thehowl]

• I don't think there is a need for checkComments? (ie. yes, always check comments)
• Why is it currently set to false in both instances we call addIssue?

[0xtekgrinder]

So, apologies but I needed a bit of time to take in this and try to come up with how I think we should move forward.

At this point, I dislike this because it requires doing a second round of parsing for every file that has a linting issue. This is slow and inefficient when the parsing already happens for the GnoVM anyway.

I think the way to go about this is actually placing comment parsing in the GnoVM. Sorry to have changed my mind on what I said, but trying to understand how the linter should work is tricky. As you may have noted, it is still pretty rudimentary and we don't many established design features.

However, I'm conflicted. In order to take advantage of parsing comments directly in the GnoVM (this would have to happen in gnolang/go2gno), we'd need a way for the linter to obtain the precise Nodes where the error occurred within the GnoVM.

I suggest you take a look at changing the recover function for Preprocess:

gno/gnovm/pkg/gnolang/preprocess.go

Lines 155 to 157 in 9734695

	defer func() {
	if r := recover(); r != nil {
	// before re-throwing the error, append location information to message.

I suggest you use last, n and ns (NodeStack, this as far as I understand should contain the current "depth" of nodes in the preprocessing tree) to find the last Stmt or Decl being processed. With that, you can put it into PreprocessError, and in conjuction with adding comments as attributes in go2gno, you should be able to avoid double-parsing the source.

I understand this may be more complicated than you're willing to take on. If you need help, please do ping me and I'll try to help you best I can. If the rabbit hole seems too big, that's OK too and we can try to take it on as the core team (sooner or later ™️).

Looking for a second pair of eyes @gfanton @ajnavarro if you see better approaches.

Thanks for the answer, to be honest I think it is a bit too in depth for my first issue and will end up taking too much of your time asking question. So I think the best is to let a core team member tackle it and I will continue to build on it after