Advance C# linter based on mcs -t:module #952

hernot · 2017-09-25T11:19:24Z

Added addtional c# linter called mcsc using the module target to not only catch syntax errors but also indicate compilation errors and possibly missing assemblies etc. the lint_file flag is set to 1 to avoid delays on large directory trees.

Still missing is a possibility to define subdirectories which should be excluded from linting.

And suggestions for improvement wellcome.

Have added a ale-cs.txt file in doc describing the existing mcs and mcsc linter. as this is not included in the files on github.

The existing c-charp linter used the --syntax check mode of the mono mcs compiler only. The new mcsc linter tries to compile the files located in a directory tree located bejond the specified source directory or the current one if no source is explicitly specified. The resulting module target is placed in a temporary file managed by ale.

w0rp

Look through the codebase and try and write some smaller and easier to read code.

You will need to write Vader tests to cover your new command callback and your error handler.

w0rp · 2017-09-25T20:26:02Z

ale_linters/cs/mcsc.vim

+    \    . ' "' . join(glob('**/*.cs',v:false,v:true),'" "') . '"'
+	exe 'cd ' . l:cwd
+	return l:cmd
+endfunction


This is some very poorly formatted code. Lose all of the type checking, and use blank lines and comments here or there.

w0rp · 2017-09-25T20:26:18Z

ale_linters/cs/mcsc.vim

+    \    . ' ' . l:assemblies
+	\    . ' -out:' . l:out
+    \    . ' -t:module'
+    \    . ' "' . join(glob('**/*.cs',v:false,v:true),'" "') . '"'


Don't use glob like this. It's painfully slow on many machines.

w0rp · 2017-09-25T20:27:42Z

doc/ale-cs.txt

+  the assembly files specified by |g:ale_cs_mcsc_assemblies| or 
+  |b:ale_cs_mcsc_assemblies|. The mcs -unsafe option is set implicitly and has
+  not to be added using |g:ale_cs_mcsc_options| or |b:ale_cs_mcsc_options|
+  variable.


The formatting of the text is very strange here. It looks like things are randomly indented. I find it hard to read.

w0rp · 2017-09-25T20:28:15Z

doc/ale-cs.txt

+  line using the -lib: flag of mcs.
+
+g:ale_cs_mcsc_assemblies                            *g:ale_cs_mcsc_assemblies*
+                                                   *b:ale_cs_mcsc_assemblies*


You need to align your help tags to the right margin.

w0rp · 2017-09-25T20:28:38Z

doc/ale-cs.txt

+  not empty the list of assemblies will be added to the mcsc command
+  line using the -r: flag of mcs. To change the search path mcs uses to 
+  locate the specified assembly files use |g:ale_cs_mcsc_assembly_path| or
+  |b:ale_cs_mcsc_assembly_path| variables


Try and explain things more succinctly, and use short sentences.

w0rp · 2017-09-25T20:28:52Z

ale_linters/cs/mcsc.vim

+\   'executable': 'mcs',
+\   'command_callback': 'ale_linters#cs#mcsc#GetCommand',
+\   'callback': 'ale_linters#cs#mcsc#Handle',
+\	'lint_file': 1


Bad indentation.

w0rp · 2017-09-25T20:29:32Z

README.md

@@ -76,7 +76,7 @@ formatting.
 | C | [cppcheck](http://cppcheck.sourceforge.net), [cpplint](https://github.com/google/styleguide/tree/gh-pages/cpplint), [gcc](https://gcc.gnu.org/), [clang](http://clang.llvm.org/), [clangtidy](http://clang.llvm.org/extra/clang-tidy/) !!, [clang-format](https://clang.llvm.org/docs/ClangFormat.html)|
 | C++ (filetype cpp) | [clang](http://clang.llvm.org/), [clangcheck](http://clang.llvm.org/docs/ClangCheck.html) !!, [clangtidy](http://clang.llvm.org/extra/clang-tidy/) !!, [cppcheck](http://cppcheck.sourceforge.net), [cpplint](https://github.com/google/styleguide/tree/gh-pages/cpplint) !!, [gcc](https://gcc.gnu.org/), [clang-format](https://clang.llvm.org/docs/ClangFormat.html)|
 | CUDA | [nvcc](http://docs.nvidia.com/cuda/cuda-compiler-driver-nvcc/index.html) |
-| C# | [mcs](http://www.mono-project.com/docs/about-mono/languages/csharp/) |
+| C# | [mcs](http://www.mono-project.com/docs/about-mono/languages/csharp/) see:`help ale-cs-mcs` for details,[mcsc](http://www.mono-project.com/docs/about-mono/languages/csharp/) see:`help ale-cs-mcsc` for details and configuration|


You need to mark this with !!, as you've set lint_file to 1.

You need to document this in doc/ale.txt too.

Implements suggestions and recommendations suggested by the first review of the "Advance C# linter based on mcs -t:module (dense-analysis#952)" pull request. - Clarifies and simplifies description of linters and options - Added links to help file and marked the mcsc linter as to be run only when file in buffer is saved or loaded. - Addec comments to the mcsc.vim file to clarify code - removed type checks considered not necessary be reviewer.

hernot · 2017-09-26T07:28:05Z

Ok i have addressed all your recommendations. The only one i at max be able to address partially, if i get some instruction and support form your side is the writing of vader tests. I do not know vader and i do have no docker setup here. So even if able to write a test, i fear I would not be able to verify that it is really testing what it should test. Any advice? Any suggestions?

Ah and the strange formatting of source and doc was due to mix of and white space, fixed.

Implements suggestions and recommendations suggested by the first review of the "Advance C# linter based on mcs -t:module (dense-analysis#952)" pull request. - Clarifies and simplifies description of linters and options - Added links to help file and marked the mcsc linter as to be run only when file in buffer is saved or loaded. - Added comments to the mcsc.vim file to clarify code - removed type checks considered not necessary be reviewer. - addresses findings by vader

w0rp

This is looking a lot better. 👍

You should write Vader tests for your new functions. Look at the tests in the test/handler and test/command_callback directories.

w0rp · 2017-09-26T08:09:18Z

ale_linters/cs/mcsc.vim

+
+    " switch back to the working directory to the path
+    " stored above.
+    exe 'cd ' . l:cwd


You shouldn't execute a command to change the working directory. Just include cd ... && in the command you want ALE to run.

w0rp · 2017-09-26T08:09:31Z

ale_linters/cs/mcsc.vim

+    " it seems not to work reliably, which is why vim built in
+    " glob function is used to collect the list of '*.cs'
+    " files.
+    let l:cmd = 'cd ' . l:base . ';'


You can just use return here instead, with the advice above.

w0rp · 2017-09-26T08:09:52Z

ale_linters/cs/mcsc.vim

+    \    . 'mcs -unsafe'
+    \    . ' ' . ale#Var(a:buffer, 'cs_mcsc_options')
+    \    . ' ' . l:path
+    \    . ' ' . l:assemblies


Try and include these additional arguments only if they aren't empty. Have a look at other linters for examples.

w0rp · 2017-09-26T08:11:06Z

ale_linters/cs/mcsc.vim

+    \    . ' ' . l:assemblies
+    \    . ' -out:' . l:out
+    \    . ' -t:module'
+    \    . ' "' . join(glob('**/*.cs',v:false,v:true),'" "') . '"'


glob will perform very badly on many machines. Can you pass just the one file instead, or something like that?

w0rp · 2017-09-26T08:11:31Z

ale_linters/cs/mcsc.vim

+    let l:cwd = expand('#' . a:buffer . ':p:h')
+    " get the base path of the source tree to lint and if
+    " not empty and exists switch working directory to it
+    let l:base = ale#Var(a:buffer,'cs_mcsc_source')


Add a space after commas, here and above.

w0rp · 2017-09-26T08:11:45Z

ale_linters/cs/mcsc.vim

+    " get the base path of the source tree to lint and if
+    " not empty and exists switch working directory to it
+    let l:base = ale#Var(a:buffer,'cs_mcsc_source')
+    if isdirectory(l:base)


Add a blank line above if statements which do not start a new block.

w0rp · 2017-09-26T08:11:57Z

ale_linters/cs/mcsc.vim

+" list of assemblies to consider
+let g:ale_cs_mcsc_assemblies = get(g:,'ale_cs_mcsc_assemblies',[])
+function! ale_linters#cs#mcsc#GetCommand(buffer) abort
+


Delete the blank line here.

w0rp · 2017-09-26T08:12:26Z

ale_linters/cs/mcsc.vim

@@ -0,0 +1,107 @@
+" general mcs options which are likely to stay constant across
+" source trees like -pkg:dotnet
+let g:ale_cs_mcsc_options = get(g:, 'ale_cs_mcsc_options', '')


You might like to use ale#Set for these variables. Look at the codebase for other examples.

hernot · 2017-09-26T09:03:43Z

Hi
I would appreciate if that workaround would not be necessary but with my installed version of mono mcs compiler the mcs compiler hang nine times out of 10 when trying to use the built in -recurse flag. If it would have worked 10 out of 10 times i would have used that instead of the glob function of vim without resorting to the cd solution if you have better idea for how to properly implement the workaround please let me know.

I decided against including the ale_cs_mcsc_source path inside the glob pattern to keep command string as short as possible even if several hundred(* files have to be included in the build.

(*)addmitted a rather poorly organized source tree and and likely indicating of design flaws, still if not all files are included in the linting command (see above) users would receive errors about missing assemblies for classes, types and namespaces defined by source files from the same source tree.

Currently i do have not the time and resources to investigating the reason for mcs hanging when using -recurse flag, in worst submitting a bugreport or even a bugfix to mono project and than adding version checks disabling linter if version with broken -recurse is installed.

hernot · 2017-09-26T10:35:01Z

What the hack, now retrying the -recurse flag every thing works as expected. No idea why, what and where the problem was. Need to keep an eye on this. Never the less for the next commit i simplified to using -recurse flag of mcs compiler. Added a NOTE to the help that the case that mcsc does not indicate errors in the currently editor is related to mcs is a multipass comiler which stops after found errors at a specific comilation stage, not advancing to the next.

Will have a look into the tests, hope i do not make things even worse than now

hernot · 2017-09-26T11:03:04Z

a question does a test_mcs_handerl.vader not exist or was it not commited as the mcs linter was already present in the github master branch. If not present at all should i also add a vader test for the mcs like i did for the help files or just for the mcsc i have added?

Implements suggestions and recommendations suggested by the first review of the "Advance C# linter based on mcs -t:module (dense-analysis#952)" pull request. - Clarifies and simplifies description of linters and options - Added links to help file and marked the mcsc linter as to be run only when file in buffer is saved or loaded. - Added comments to the mcsc.vim file to clarify code - removed type checks considered not necessary be reviewer. - addresses findings by vader - implements tests for mcs.vim and mcsc.vim linter

hernot · 2017-09-26T14:24:47Z

Ok added the tests for mcs.vim which was missing them in the master tree and for mcsc.vim
The continuous-integration/travis-ci/pr test seems from what i can see fail cause i have inserted mcsc in ale-vim.txt file. not sure. please check your self and let me know if that would be the only reason.

Implements suggestions and recommendations suggested by the first review of the "Advance C# linter based on mcs -t:module (dense-analysis#952)" pull request. - Clarifies and simplifies description of linters and options - Added links to help file and marked the mcsc linter as to be run only when file in buffer is saved or loaded. - Added comments to the mcsc.vim file to clarify code - removed type checks considered not necessary be reviewer. - addresses findings by vader - removed call to getcwd and cd in vim script - handler expands file names relative to route of source tree into absolute pathes. Fixes errors not being marked when vim is started from subdirectory of source tree. - implements tests for mcs.vim and mcsc.vim linter

hernot · 2017-09-26T20:12:05Z

Finally, fixe some nasty behavior, when vim is not run from base directory of source tree.

w0rp

This is better without the glob() call now, which could execute pretty slowly.

See my comments on the tests. Assertions for exact string matches will be better.

w0rp · 2017-09-26T22:36:27Z

test/command_callback/test_cs_mcsc_command_callbacks.vader

+
+  let b:command = ale_linters#cs#mcsc#GetCommand(bufnr(''))
+
+  Assert match( b:command, '^\s*cd\s\+"."\s*;\s*mcs\s\+-unsafe\s\+-out:\S\+\s\+-t:module\s\+-recurse:"\*\.cs"\s*$' ) == 0,


The regular expressions here are very hard to read, so it's hard to make sense of what the tests are testing. I recommend writing tests for exact strings with AssertEqual instead. You will probably need to build certain parts of the strings base on the filename, etc.

Is in this case not possible cause the GetCommand registers a temporary file with ale, which differs at every call so the assert is the only possibility to make test succeed, unless in your test machinery you have an AssertFunction or some other function which helps to handle exactly that case.

Please any advice.

I recommend replacing just the temporary file in the actual string with some other sequence of characters. Then you can use an exact string for the expected string.

You could substitute -out:[^ ]* or something like that with -out:TEMP or just anything really.

you mean

let b:command = subsitute(b:command,'/\+tmp/\+\S\+','/tmp/some_tmp_file.tmp')

and than use this modified string in the AssertEqual

Yeah, something like that. But bear in mind that the directory will be different from /tmp on Mac and Windows, so you should probably search for some characters after '-out:'.

Sure i do. I finish the tests and the fixes commit and as soon as they are working properly i let you know

w0rp · 2017-09-26T22:37:10Z

ale_linters/cs/mcsc.vim

+\   'executable': 'mcs',
+\   'command_callback': 'ale_linters#cs#mcsc#GetCommand',
+\   'callback': 'ale_linters#cs#mcsc#Handle',
+\    'lint_file': 1


The indentation is wrong here.

w0rp · 2017-09-26T22:37:25Z

ale_linters/cs/mcsc.vim

+    " path and not just the file loaded in the buffer
+    let l:pattern = '^\(.\+\.cs\)(\(\d\+\),\(\d\+\)): \(.\+\): \(.\+\)'
+    let l:output = []
+    let l:source = ale#Var(a:buffer,'cs_mcsc_source')


Add a single space after commas.

w0rp · 2017-09-26T22:38:09Z

ale_linters/cs/mcsc.vim

+    " if list of assemblies to link is not empty convert it to the
+    " appropriate -r: parameter of mcs
+    let l:assemblies = ale#Var(a:buffer,'cs_mcsc_assemblies')
+    if !empty(l:assemblies)


Put a single blank line before any if statement which doesn't start a new level of indentation.

w0rp · 2017-09-27T11:09:10Z

test/command_callback/test_cs_mcsc_command_callbacks.vader

+
+  let b:command = ale_linters#cs#mcsc#GetCommand(bufnr(''))
+  let b:command = substitute(b:command, '-out:' . g:temppathpattern, '-out:' . g:sometempfile, '')
+  let b:command = substitute(b:command, '\s\+', ' ', 'g')


Add unlet! b:command to your After block above. Otherwise this variable might end up being used in some other test.

w0rp · 2017-09-27T11:10:28Z

test/command_callback/test_cs_mcsc_command_callbacks.vader

+  let g:ale_cs_mcsc_options = '-pkg:dotnet'
+
+  let b:command = ale_linters#cs#mcsc#GetCommand(bufnr(''))
+  let b:command = substitute(b:command, '-out:' . g:temppathpattern, '-out:' . g:sometempfile, '')


sometmpefile will cause problems here with \ in paths on Windows. I recommend using something simpler like '-out:TEMP_FILE' and testing for that. You're replacing it anyway.

Implements suggestions and recommendations suggested by the first review of the "Advance C# linter based on mcs -t:module (dense-analysis#952)" pull request. - Clarifies and simplifies description of linters and options - Added links to help file and marked the mcsc linter as to be run only when file in buffer is saved or loaded. - Added comments to the mcsc.vim file to clarify code - removed type checks considered not necessary be reviewer. - addresses findings by vader - removed call to getcwd and cd in vim script - handler expands file names relative to route of source tree into absolute pathes. Fixes errors not being marked when vim is started from subdirectory of source tree. - implements tests for mcs.vim and mcsc.vim linter

hernot · 2017-09-27T11:26:27Z

Ok all fixed.

Also removed Assert match in ale_cs_mcs_command_callbacks.vader using a substitute to condense multiple whithespaces into a single space char.

w0rp · 2017-09-27T22:22:15Z

Check out the results in Travis CI. doc/ale.txt and README.md are out of sync.

hernot · 2017-09-28T07:04:05Z

The not ins sync problem is not that the tables are not in sync, it is just that the test script stumbles over the greediness of regular expressions in line

53 | sed 's/see.*(,|$)/\1/g' \

This removes in the following line everything starting at the first see till to the end of the line

| C# | mcs see:help ale-cs-mcs for details, mcsc !! see:help ale-cs-mcsc for details and configuration|

with the consequence that only mcs is reported.

I suggest to improve line 53 of the test script as follows

53 | sed 's/see[^,]*(,|$)/\1/g' \

With this change both tables appear in sync as are. Further down in the README.MD sombody solved the same problem by putting the see of the first entry between (). But is that the desired solution as the the () are already used to indicate the links to the compiler, linter project pages. Further it is not consistent with any other see following the last entry of the lists eg.

| Kotlin | kotlinc !!, ktlint !! see :help ale-integration-kotlin for configuration instructions

There is no () around the see. What would be the conformant formatting of the table you suggest markdown would require. In any case i would recommend the fix for properly handling of the greediness of regular expressions.

Similra for Rust

the cargo linter is not embraced in []

w0rp · 2017-09-28T12:28:55Z

Okay, makes sense. I'll fix it on master.

w0rp · 2017-09-28T12:30:02Z

Cheers! 🍻

w0rp · 2017-09-28T12:32:20Z

I implemented the regex you suggested. I think that'll do the trick for now.

hernot added 2 commits September 25, 2017 12:54

w0rp requested changes Sep 25, 2017

View reviewed changes

hernot force-pushed the master branch from 88f020d to ac1d6bd Compare September 26, 2017 07:53

w0rp requested changes Sep 26, 2017

View reviewed changes

hernot force-pushed the master branch from 58f073a to e7c32e9 Compare September 26, 2017 14:20

hernot force-pushed the master branch from 8b97fb3 to e276951 Compare September 26, 2017 19:48

hernot force-pushed the master branch from 9e3ead2 to ab590ed Compare September 26, 2017 20:00

w0rp requested changes Sep 26, 2017

View reviewed changes

w0rp requested changes Sep 27, 2017

View reviewed changes

hernot force-pushed the master branch from eff77d1 to 8f6044b Compare September 27, 2017 11:21

w0rp approved these changes Sep 28, 2017

View reviewed changes

w0rp merged commit 9fc01bd into dense-analysis:master Sep 28, 2017


		let b:command = ale_linters#cs#mcsc#GetCommand(bufnr(''))

		Assert match( b:command, '^\scd\s\+"."\s;\smcs\s\+-unsafe\s\+-out:\S\+\s\+-t:module\s\+-recurse:"\\.cs"\s*$' ) == 0,

Advance C# linter based on mcs -t:module #952

Advance C# linter based on mcs -t:module #952

Conversation

hernot commented Sep 25, 2017

w0rp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hernot commented Sep 26, 2017 • edited

w0rp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hernot commented Sep 26, 2017

hernot commented Sep 26, 2017

hernot commented Sep 26, 2017

hernot commented Sep 26, 2017

hernot commented Sep 26, 2017

w0rp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hernot Sep 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hernot Sep 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

w0rp Sep 27, 2017 • edited

Choose a reason for hiding this comment

hernot commented Sep 27, 2017 • edited

w0rp commented Sep 27, 2017

hernot commented Sep 28, 2017 • edited

w0rp commented Sep 28, 2017

w0rp commented Sep 28, 2017

w0rp commented Sep 28, 2017

hernot commented Sep 26, 2017 •

edited

hernot Sep 27, 2017 •

edited

hernot Sep 27, 2017 •

edited

w0rp Sep 27, 2017 •

edited

hernot commented Sep 27, 2017 •

edited

hernot commented Sep 28, 2017 •

edited