Stylecheck script fails on MacOS #13492

[hiroshitashir]

• Install GNU grep on macOS with brew install grep
• Use GNU grep with -E option
• The following errors fixed.

$scripts/check_style.sh
ggrep: warning: stray \ before /
ggrep: warning: stray \ before /
ggrep: warning: * at start of expression
ggrep: warning: * at start of expression

The change was tested on macOS and Linux

Fixes #13492

[github-actions]

Thank you for your contribution to the Solidity compiler! A team member will follow up shortly.

If you haven't read our contributing guidelines and our review checklist before, please do it now, this makes the reviewing process and accepting your contribution smoother.

If you have any questions or need our help, feel free to post them in the PR or talk to us directly on the #solidity-dev channel on Matrix.

[hiroshitashir]

I believe these are valid style errors. '*'s should be left-aligned.

Coding style error:
libsolidity/analysis/ControlFlowBuilder.cpp:587:	if (auto const *builtinFunction = m_inlineAssembly->dialect().builtin(_functionCall.functionName.name))
libsolidity/analysis/ControlFlowBuilder.h:155:	void placeAndConnectLabel(CFGNode *_node);
libsolidity/analysis/TypeChecker.cpp:1556:			if (auto const *variableDeclaration = dynamic_cast<VariableDeclaration const*>(identifier->annotation().referencedDeclaration))
libsolidity/codegen/CompilerContext.h:376:	CompilerContext *m_runtimeContext;
libsolidity/codegen/ExpressionCompiler.cpp:2231:	ArrayType const *arrayType = dynamic_cast<ArrayType const*>(&baseType);
libyul/backends/evm/StackLayoutGenerator.cpp:350:			CFG::BasicBlock const *block = *toVisit.begin();

If so, can I change files above and change right aligned '*'s to left aligned in the same PR?

[aarlt]

Hey @hiroshitash!

Thank you very much for your contribution!

What is the MacOS version that youre using? I was just checking the current behavoir on my machine (running MacOS Ventura 13.4.1) and it seem to ship a GNU compatible grep (grep --version returns grep (BSD grep, GNU compatible) 2.6.0-FreeBSD.). At least what I can tell is that the current version of the script running on MacOS v13.4.1 seem to have exactly the same behaviour of your changes. Can you run grep --version on your machine? I think -E is a GNU extension to grep, where at least the version string of grep on MacOS 13.4.1 is explicitly stating that it is GNU compatible. Can you also verify that youre using the grep
shipped with MacOS?

However, your changes look good for me. But at least for me it looks like that these changes are not needed - at least from MacOS version 13.4.1 on.

[hiroshitashir]

Hello @aarlt,

Absolutely! I am running the same version of MacOS (Ventura 13.4.1) with grep (BSD grep, GNU compatible) 2.6.0-FreeBSD.

# default grep on MacOS
 % grep --version
grep (BSD grep, GNU compatible) 2.6.0-FreeBSD

# installed GNU grep
 % ggrep --version
ggrep (GNU grep) 3.11
Packaged by Homebrew

Although BSD grep says it's compatible with GNU grep, it gives an error with a command below (while GNU grep doesn't)

# BSD grep shows an error
 % grep -E -v -e "return [*]" -e "^* [*]" -e "^*//.*"
grep: repetition-operator operand invalid

# GNU grep shows no errors, but some warnings
% ggrep -E -v -e "return [*]" -e "^* [*]" -e "^*//.*"
ggrep: warning: * at start of expression
ggrep: warning: * at start of expression

# GNU grep shows no warnings with modified logic
% ggrep -E -v -e "return [*]" -e ":\s*[*]" -e ".*//.*"

Please see the conversation here for more information.

One question: With my modification, style check script still fails with legitimate style errors here. Should I modify right-aligned '*'s in the files?

[nikola-matic]

I believe these are valid style errors. '*'s should be left-aligned.

Coding style error:
libsolidity/analysis/ControlFlowBuilder.cpp:587:	if (auto const *builtinFunction = m_inlineAssembly->dialect().builtin(_functionCall.functionName.name))
libsolidity/analysis/ControlFlowBuilder.h:155:	void placeAndConnectLabel(CFGNode *_node);
libsolidity/analysis/TypeChecker.cpp:1556:			if (auto const *variableDeclaration = dynamic_cast<VariableDeclaration const*>(identifier->annotation().referencedDeclaration))
libsolidity/codegen/CompilerContext.h:376:	CompilerContext *m_runtimeContext;
libsolidity/codegen/ExpressionCompiler.cpp:2231:	ArrayType const *arrayType = dynamic_cast<ArrayType const*>(&baseType);
libyul/backends/evm/StackLayoutGenerator.cpp:350:			CFG::BasicBlock const *block = *toVisit.begin();

If so, can I change files above and change right aligned '*'s to left aligned in the same PR?

Yes please. Not really sure how these weren't caught by the script in the first place (or subsequently). Also, please rebase - I made slight changes to the check style script to have it fail when using namespace std; is used in specified directories.

[hiroshitashir]

Hi @aarlt @nikola-matic,
I rebased and squashed my old and new changes into one commit.

I also made one additional change besides fixing right-aligned '*'s:

• \s was replaced with [[:space:]] in scripts/check_style.sh as git grep on my MacOS did not behave correctly with \s (It looks like old versions of grep do not behave correctly with \s according to here).

All tests passed, but please let me know if anything doesn't look right.

[aarlt]

Hey @hiroshitashir!
I'm very sorry for the very long reviewing delay! I think that your PR is good! If you're still arround, can you probably do a rebase?
Thank you very much for your contribution!

[hiroshitashir]

Hello @aarlt,

I just rebased my branch and all tests passing. Let me know if there is anything else I can do.

[cameel]

A style check script should absolutely not modify your system by installing new software. If it requires something and it's not available, it just fail.

We actually added a portable grep helper recently, so please source common.sh and use it:

solidity/scripts/common.sh

Lines 317 to 326 in fbbac9c

	function gnu_grep
	{
	if [[ "$OSTYPE" == "darwin"* ]]; then
	command_available ggrep --version
	ggrep "$@"
	else
	command_available grep --version
	grep "$@"
	fi
	}

[aarlt]

Thanks @cameel! You're right, like that it is much cleaner! Sorry for the confusion @hiroshitashir, but the proposed changes make a lot of sense. Let me know if you have any questions.

[nikola-matic]

I'll be closing this in lieu of our current work on introducing clang-format, which essentially do away with all of the portability issues (at least as far as the check_style script is concerned).