athena-framework / athena

An ecosystem of reusable, independent components
https://athenaframework.org
MIT License
211 stars 17 forks source link

Audit compile time errors #280

Open Blacksmoke16 opened 1 year ago

Blacksmoke16 commented 1 year ago

With the introduction of https://github.com/crystal-lang/crystal/pull/13260 and https://github.com/crystal-lang/crystal/pull/13261, compile time error messages should now properly point to the related node. Up until now, Athena has been raising somewhat more verbose error messages than normal in order to help the user understand the error/where the problem is. For example:

# @[ADI::Register]
struct ShoutTransformer
  def transform(value : String) : String
    value.upcase
  end
end

@[ADI::Register(public: true)]
struct SomeAPIClient
  def initialize(@transformer : ShoutTransformer); end
end

ADI.container.some_api_client

If a service dependency is unable to be resolved, the error message would be like:

$ crystal run src/components/dependency_injection/test.cr 
Showing last frame. Use --error-trace for full trace.

There was a problem expanding macro 'finished'

Code in src/components/dependency_injection/src/service_container.cr:445:3

 445 | macro finished
       ^
Called macro defined in src/components/dependency_injection/src/service_container.cr:445:3

 445 | macro finished

Which expanded to:

 > 6 |     
 > 7 | 
 > 8 |     include ResolveArguments
           ^
Error: Failed to auto register service 'some_api_client'.  Could not resolve argument 'transformer : ShoutTransformer'.

Whereas after those PRs are merged, it'll be like:

$ ccrystal run src/components/dependency_injection/test.cr 
Using compiled compiler at /home/george/dev/git/crystal/.build/crystal
Showing last frame. Use --error-trace for full trace.

In src/components/dependency_injection/test.cr:12:18

 12 | def initialize(@transformer : ShoutTransformer); end
                     ^----------
Error: Failed to auto register service 'some_api_client'.  Could not resolve argument 'transformer : ShoutTransformer'.

Now that it actually points to the parameter that was unable to be resolved, it doesn't make sense to also include it in the error message.

I'm sure there are other cases where the message could be revised based on the better error traces. We should also make sure the compile time errors are pointing to the proper node as well. Otherwise it could make figuring out the cause even more confusing.