search

dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
19.03k stars 4.03k forks source link

IDE is using wrong default checksum (Sha1) and compiler switched to Sha256 #43921

Open KirillOsenkov opened 4 years ago

KirillOsenkov commented 4 years ago

SourceText.From factory methods are still defaulting to checksum Sha1: http://sourceroslyn.io/#Microsoft.CodeAnalysis/Text/SourceText.cs,d444ba9c80426d47

Meanwhile the compiler has switched to Sha256: https://github.com/dotnet/roslyn/pull/24820

Also there's a disconnect that the compiler does not respect the user-specified checksum and hardcodes Sha256: https://github.com/dotnet/roslyn/issues/24735

This can result in confusion as the checksum algorithm specified by the workspace host doesn't matter.

@tmat @gafter

tmat commented 4 years ago

We can't change defaults used by APIs. That would be a breaking change.

KirillOsenkov commented 4 years ago

Curious about an actual sample scenario that would break.

Neme12 commented 1 year ago

We can't change defaults used by APIs. That would be a breaking change.

Wouldn't this an acceptable breaking change? I don't really expect people who don't explicitly specify a checksum algorithm to actually depend on it or use it.

This seems like a really small and low-risk breaking change that would be OK for a major release version. And it wouldn't be a binary breaking change - users would have to recompile with the new version to get the new checksum algorithm.

Neme12 commented 1 year ago

Wasn't the compiler changing its default hash algorithm to sha256 technically a breaking change too? And that change was still taken.

CyrusNajmabadi commented 1 year ago

I don't see any reason to make a breaking change. At best, no one is harmed. But at worse, it causes pain for some users.

I don't see what benefit we get from breaks here at all.

Neme12 commented 1 year ago

Well, the reason would be that sha1 is officially deprecated. And the compiler already uses sha256 by default anyway.

CyrusNajmabadi commented 1 year ago

Well, the reason would be that sha1 is officially deprecated.

I don't see what customer benefits from this. It how it benefits us. Breaking things needs avery strong justification as to the value it provides.

Right now, I'm not actually seeing any value. We also haven't gotten reports or upvotes from customers asking for this.

So I repeat that at best this is neutral, but at worse this hurts people.

And the compiler already uses sha256 by default anyway.

What the compiler does isn't relevant. They have different customers and different ship scenarios and rules they have to operate under. For example, they might have to operate under certain constraints that make things a requirement (fips, etc.).

Absent a requirement that we just remove sha1, or lots of customers indicating this default is a substantive problem, we should not arbitrarily break people :-)

Neme12 commented 1 year ago

I just have a hard time seeing how this would break anyone.

Neme12 commented 1 year ago

What the compiler does isn't relevant. They have different customers and different ship scenarios and rules they have to operate under.

Compared to who/what? We are talking about the compiler.

CyrusNajmabadi commented 1 year ago

I just have a hard time seeing how this would break anyone.

Anyone using our apis and storing things based on hashes.

CyrusNajmabadi commented 1 year ago

Compared to who/what? We are talking about the compiler.

Compared to consumers using the API who are not the compiler :-)