Solidity support revisited #1843
Conversation
There was a problem hiding this comment.
Thank you @wmitsuda!
I still found a few things which have to be done, so please take a look at my comments.
| { | ||
| pattern: /\bmapping\s*\(.*\s+=>\s+.*\)(?:\s+(?:private|public|internal|external|inherited))?\s+[A-Za-z_]\w*\b/, | ||
| inside: { | ||
| keyword: { |
There was a problem hiding this comment.
Missing single quotes.
| keyword: { | |
| 'keyword': { |
| } | ||
| }, | ||
| { | ||
| pattern: /\bmapping\s*\(.*\s+=>\s+.*\)(?:\s+(?:private|public|internal|external|inherited))?\s+[A-Za-z_]\w*\b/, |
There was a problem hiding this comment.
I have a problem with .* as the dot also includes most of \s+=>\s+ which invites catastrophic backtracking.
What exactly is .* supposed to match?
There was a problem hiding this comment.
The first .* is supposed to be a builtin type, the second one may be any type, even another mapping. Don't know if there is a better way to express this.
There was a problem hiding this comment.
For more information, this is the language specification: https://solidity.readthedocs.io/en/v0.5.7/types.html#mapping-types
There was a problem hiding this comment.
I don't what the most complex type name could be but wouldn't \w+ instead of .* also work? So \(\s*\w+\s*=>\s*\w+\s*\).
And if we were to allow any content, then \([^()]*\) might be what we want.
There was a problem hiding this comment.
It is possible to have a mapping of mapping, for e.g.: mapping(address => mapping(uint => customStruct[])) someName, and so on... how can I model this recursion on prism? Is there some good example I can follow?
There was a problem hiding this comment.
Matching such expression correctly requires a context-free grammar, so we can't match them with regexes. It's fundamentally impossible.
But we can approximate by matching a limited number of recursion levels.
E.g. \((?:[^()]|\((?:[^()]|\([^()]*\))*\))*\) will match recusive bracket expression for up to a maximum depth of 3.
This is how we usually deal with nested expression.
| pattern: /\b(?:address|string|bytes\d*|int\d*|uint\d*|bool|u?fixed\d+x\d+)\b(?:\s+(?:indexed\s+)?[A-Za-z_]\w*\s*[,)])?/, | ||
| inside: { | ||
| 'attr-name' : { | ||
| pattern: /\b(address|string|bytes\d*|int\d*|uint\d*|bool|u?fixed\d+x\d+)\b(?:\s+(?:indexed\s+)?[A-Za-z_]\w*\s*[,)])?/, |
There was a problem hiding this comment.
I'm not really sure but isn't (?:indexed\s+)? supposed to be part of the lookbehind?
Also, shouldn't \s*[,)] be a lookahead rather than being part of the name?
Why is the whole non-lookbehind expression optional? This might cause empty tokens.
There was a problem hiding this comment.
Actually, this part is from the original patch from the original author, have to admit I didn't pay much attention to it. I think the original author made a mistake. Not sure exactly what besides a simple attribute declaration he intended to match. I'll remove what I think is unnecessary to make it work and update the patch.
| pattern: /\b(?:contract|interface|library|using|struct|function|modifier)\s+[A-Za-z_]\w*(?:\s+is\s+(?:[A-Za-z_][,\s]*)*)?\b/, | ||
| inside: { | ||
| 'variable': { | ||
| pattern: /\b(contract|interface|library|using|struct|function|modifier)\s+[A-Za-z_]\w*(?:\s+is\s+(?:[A-Za-z_][,\s]*)*)?\b/, |
There was a problem hiding this comment.
Please move the \s+ inside the lookbehind. Tokens should contain leading or trailing spaces.
| pattern: /\b(contract|interface|library|using|struct|function|modifier)\s+[A-Za-z_]\w*(?:\s+is\s+(?:[A-Za-z_][,\s]*)*)?\b/, | |
| pattern: /(\b(?:contract|interface|library|using|struct|function|modifier)\s+)[A-Za-z_]\w*(?:\s+is\s+(?:[A-Za-z_][,\s]*)*)?\b/, |
|
Any plan to merge this PR? Is the last blocker to migrate from highlight.js to Prism for us. |
|
Since the author never got to make the requested changes, I can make the required changes within the next few days. |
Fixes #1404 . Revised PR from #1406