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.

Sign up for GitHub

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

Jump to bottom

Remove unsafe FromJSON Name instance #834

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

Remove unsafe FromJSON Name instance #834

wants to merge 2 commits into from

Conversation

georgefst
Copy link
Contributor

No description provided.

@georgefst georgefst requested a review from a team January 17, 2023 11:11
@georgefst georgefst force-pushed the georgefst/remove-fromjson-name branch 2 times, most recently from 3785868 to a313773 Compare January 17, 2023 11:30
@dhess
Copy link
Member

dhess commented Jan 17, 2023

Per comments in chat, we're doing this now because a) we're about to change Primer.Name.Name to be a smart constructor, but we want to use yet another, more strict validation type for communicating across JSON APIs; and b) because the FromJSON instance would leave a backdoor of sorts to get around the smart constructor.

@georgefst
fix: Remove unsafe FromJSON Name instance
d24cca3 
Because it used `deriving newtype`, this instance constructed names without any validation, similarly to `unsafeMkName`. While we don't currently have any name validation, we expect that we will soon move to a "smart constructor" approach, ripping out most uses of `unsafeMkName`. We don't then want `fromJSON @Name` to remain as a validation-skipping backdoor.

An alternative would be to use our smart constructor (i.e. `safeMkName`) in a manual implementation of `fromJSON`. But given that the instance is unused (other than to define more unused instances for types which contain `Name`), we may as well just remove it.

N.B. This instance has been around since our old prototype frontend, and may have been useful back then.
@georgefst
undo - total hack!
c93e316
@georgefst georgefst force-pushed the georgefst/remove-fromjson-name branch from a313773 to c93e316 Compare January 17, 2023 12:27
@georgefst
Copy link
Contributor Author

Right, this got bigger than expected. HLS didn't report all the knock-on errors straight away, which made me think the scope was smaller.

The second commit highlights the handful of places where it isn't obvious how to proceed. It might make sense to now remove even more FromJSON instances, seeing as we've already got rid of so many, and they seem like Vonnegut relics, and may effect compilation times. But then I don't know how we'd define instance DBType App. It might be easiest to use the smart constructor in defining the Name instance and keep all the other instances.

Anyway, since this isn't urgent, I'm going to leave it here for now. Maybe @dhess will want to take it over as part of the move towards adding name validation, and/or maybe it should be discussed at our next developer meeting.

@georgefst georgefst marked this pull request as draft January 17, 2023 13:19
@georgefst
Copy link
Contributor Author

Related: #125.

Also, note that #837 potentially introduces another similar backdoor via Read.

@brprice
Copy link
Contributor

brprice commented Feb 2, 2023

Also, note that #837 potentially introduces another similar backdoor via Read.

I suspect this will actually end up in #863 (and may be expanded in some other benchmark-by-replay PR, possibly #837 itself)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@georgefst @dhess @brprice