Add column information inside ERB templates #46171
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We are currently mutating exception objects and I would like to stop doing that. Unfortunately the views are calling many methods directly on the exception and expecting that the mutations exist. This patch refactors the templates so that they ask the ExceptionWrapper class for information about the exception rather than directly asking the exception object itself
This commit adds a SyntaxErrorProxy object to active support and wraps syntax error exceptions with that proxy object. We want to enhance syntax errors with information about the source location where they actually happened (normally the backtrace doesn't contain such info). Rather than mutating the original exception's backtrace, this wraps it with a proxy object. Eventually we will implement backtrace_locations on the proxy object so that the exception handling middleware can be updated to _only_ deal with backtrace_locations and never deal with raw `backtrace`
We should get out of the business of parsing backtraces and only use backtrace locations. Backtrace locations have the file and line number information baked in, so we don't need to parse things anymore
This way we can do special stuff when the exceptions come from special locations
This commit maps the column information returned from ErrorHighlight in to column information within the source ERB template. ErrorHighlight only understands the compiled Ruby code, so this commit adds a small translation layer that converts the values from ErrorHighlight in to the right values for the ERB source template
tenderlove
force-pushed
the
refactor-errors
branch
from
db0454b
to
cd48642
Compare
We have access to the path from the backtrace location object. If we use the path of the ERB as the key, then anytime the ERB changes it'll just overwrite that template instance in the error handling hash
This isn't as easy, but should eliminate any memory leaks in dev
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR builds on the work done in #45818. It adds column information to exceptions that occur inside ERB templates. Here are some "before" and "after" screenshots.
Before:
After:
In order to accomplish this, I changed the error handling internals to always deal with backtrace locations. Before this PR, the exception wrapper class would sometimes use
backtrace_locationsand sometimes usebacktrace.ErrorHighlightonly works with backtrace locations objects, so I refactored the code to only use backtrace locations objects.Unfortunately that refactoring process was somewhat difficult due to the way we handle
SyntaxErrorexceptions.SyntaxErrorexceptions do not contain the location of the syntax error in the backtrace. Because of that, we would mutate the backtrace. We need to maintain the sameSyntaxErrorbehavior, but mutating the backtrace doesn't seem like a good idea, so this PR introduces aSyntaxErrorwrapper object that implementsbacktraceandbacktrace_locations.The next challenge is that
ErrorHighlightwill only work with Exceptions and withThread::Backtrace::Locationobjects. And clearly the above code introduces backtrace location objects that aren't instances ofThread::Backtrace::Location. I didn't want to dois_a?checks in the exception wrapper code, so I added a monkeypatch toThread::Backtrace::Locationthat returns the spot information fromErrorHighlightif it's available. This lets us do something special in theSyntaxErrorcase, though it doesn't right now.Now that actual backtrace location objects are returned,
ErrorHighlightworked with ERB, but it would report the compiled ERB source rather than the template source. For example we'd see this, which is wrong:To deal with this, I added a global hash table that maps method names to template objects here. When the exception wrapper gets the backtrace, it checks if the method was a "template" method and returns a special backtrace object. The special backtrace object simply asks the template to map the ErrorHighlight source to the actual source. The template object just turns around and asks the handler to do the mapping. The handler will be the actual template strategy (ERB, HAML, etc).
ERB translation happens here. It's kind of hacky but seems to work.
There's a few things I need to do before I consider this PR mergeable:
I need to add tests for the ERB parsing code I added. I have the tests in a separate file, just haven't put them in this PR yet.TheERROR_HANDLERShash is probably a memory leak in development. I'm not sure how to handle that yet.I'm not sure if there are any "integration" type tests with the column info from ERB. I need to add those.I think there's some cosmetic stuff I need to clean up in the code.I'd really appreciate help with the second item.