Skip to content

Conversation

@edgarfgp
Copy link
Contributor

Fixes one the multiple missing error reporting in #13147
This PR adds:

  • New compiler error when using constructor arguments on static classes [<Sealed; AbstractClass>]
  • The error is generic enough that we can reuse it for the rest of missing reporting

I will adding more error reporting in the future :)

@edgarfgp
Copy link
Contributor Author

@T-Gro Any idea why the CI is failing ? . It seem to be related to my changes but Im failing to understand why :(

@T-Gro
Copy link
Member

T-Gro commented Dec 29, 2022

@T-Gro Any idea why the CI is failing ? . It seem to be related to my changes but Im failing to understand why :(

Will have a look

@T-Gro
Copy link
Member

T-Gro commented Dec 29, 2022

@T-Gro Any idea why the CI is failing ? . It seem to be related to my changes but Im failing to understand why :(

Made a few pushes already:

  • Repro test
  • Make it work for langversion 7.0 (taking your new code path only when really needed), still fails for preview
  • Now the issue is isolated, I think it is the call to TcAttributes -> it is used "too early".
    In a mutually recursive type definition, type-checking the attribute might fail if the rest of mutually recursive definition has not been processed yet. This is exactly the case: [<DebuggerTypeProxyAttribute(typedefof<{some type defined later...}<_>>)>]

@T-Gro
Copy link
Member

T-Gro commented Dec 29, 2022

@edgarfgp Check the current code and commit log.
Possibilities I see:

  • Do your custom defensive "TcAttributes", that will only search for known built-in ones and not eagerly typecheck, especially not typechecking arguments of attributes
  • OR; move the entire logic to a later phase, where type-checked attributes already exist.

We can chat on Slack if you want to discuss this

@edgarfgp
Copy link
Contributor Author

@edgarfgp Check the current code and commit log. Possibilities I see:

  • Do your custom defensive "TcAttributes", that will only search for known built-in ones and not eagerly typecheck, especially not typechecking arguments of attributes
  • OR; move the entire logic to a later phase, where type-checked attributes already exist.

We can chat on Slack if you want to discuss this

Thanks @T-Gro :) .

@edgarfgp edgarfgp marked this pull request as ready for review January 2, 2023 21:51
@edgarfgp edgarfgp requested a review from a team as a code owner January 2, 2023 21:51
@edgarfgp
Copy link
Contributor Author

edgarfgp commented Jan 3, 2023

@T-Gro I found a later phase to do the attribute checking (Thanks for your help). Now that the CI is happy, I wanted to double-check if that error message and the ranges are clear enough. Any feedback is welcome :)

@T-Gro
Copy link
Member

T-Gro commented Jan 3, 2023

Left a suggestion.
I think we can leave certain nouns english and having rest of the sentence localized.

I would rather avoid putting verbs (are,is,..) into in and want to have them as part of the template, because different languages have different word order rules as well as negation rules. As long as it is the template, translator can change both the sentence as well as placement of the '%s' expression.

@vzarytovskii
Copy link
Member

As discussed on slack, we should probably use different errors for each case, as oppose to having one interpolated (which won't go well with localizations).

@edgarfgp edgarfgp requested a review from T-Gro January 4, 2023 08:57
@edgarfgp
Copy link
Contributor Author

edgarfgp commented Jan 4, 2023

@vzarytovskii Updated as discussed in Slack :)

T-Gro
T-Gro previously approved these changes Jan 4, 2023
@edgarfgp
Copy link
Contributor Author

edgarfgp commented Jan 5, 2023

@0101 by Any chance I have another review and hopefully merge it? So I can continue adding errors on static classes?

Copy link
Contributor

@0101 0101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. But I think we should fix the capitalization in the error messages.

@edgarfgp edgarfgp requested a review from T-Gro January 5, 2023 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants