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

Separate new-syntax contracts with linebreaks #346

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

0xEAB
Copy link
Contributor

@0xEAB 0xEAB commented Nov 10, 2023

Separate new-syntax contracts with linebreaks.
Implements #331 in a mostly unintrusive way (as far as I can tell).

@deadalnix
Copy link
Contributor

The sensible way to do this would probably to make this a list, but lists are nasty. Manually adding the line break doesn't really cut it, because when you start breaking, you want to either break the initial contract or to align the break on the initial contract.

Plus, this can be mixed up with a template constraints, attribute and qualifier, etc... This is not something I think we should add special case for, because special cases will call for special cases.

The way to go clearly is to use the list formatting, but I'm not sure how easy or hard this is. I suspect it might be hard. Also on a side note, I plan on revamping the way list work int he formatter, because in addition to being nasty, they have some pathological performance characteristics in extreme cases (like say, an array literal with thousands of elements in it). the root of the issue is that the formatter is actually completely unaware of lists;, they are implemented via spans, so it has to try all kind of combinations of break that the span is going to subsequently shut down.

@0xEAB
Copy link
Contributor Author

0xEAB commented Nov 11, 2023

This is not something I think we should add special case for

From what I’ve seen while implementing this, we’ll probably need a more sophisticated approach then.
Without special-casing this to “multiple new-syntax contracts” one’d end up with very intrusive reformatting.

@maxhaton
Copy link
Collaborator

maxhaton commented Dec 1, 2023

The sensible way to do this would probably to make this a list, but lists are nasty. Manually adding the line break doesn't really cut it, because when you start breaking, you want to either break the initial contract or to align the break on the initial contract.

Plus, this can be mixed up with a template constraints, attribute and qualifier, etc... This is not something I think we should add special case for, because special cases will call for special cases.

The way to go clearly is to use the list formatting, but I'm not sure how easy or hard this is. I suspect it might be hard. Also on a side note, I plan on revamping the way list work int he formatter, because in addition to being nasty, they have some pathological performance characteristics in extreme cases (like say, an array literal with thousands of elements in it). the root of the issue is that the formatter is actually completely unaware of lists;, they are implemented via spans, so it has to try all kind of combinations of break that the span is going to subsequently shut down.

This would be good dogfood for improving that kind of feature. This PR is quite ugly both in terms of aesthetics and upsetting grug e.g. a counter inside the parser is not great.

For the simple cases of lists I think you might be able to get a very long way with a good heuristic as long as you can prepare the head and tail right e.g. if you can get right rhythm going then I think you could lay out the bulk of the list (e.g. consider the case of lots of an array of struct initializers) geometrically rather than by cost.

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.

3 participants